Re: [Xen-devel] [libvirt test] 116216: regressions - FAIL

[Jim Fehlig]

On 11/16/2017 11:51 AM, Julien Grall wrote:

Hi all,

On 16 November 2017 at 09:40, osstest service owner
 wrote:

flight 116216 libvirt real [real]
http://logs.test-lab.xenproject.org/osstest/logs/116216/

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
  build-i386-libvirt6 libvirt-buildfail REGR. vs. 115476
  build-armhf-libvirt   6 libvirt-buildfail REGR. vs. 115476


This is failing with:

xenconfig/xen_xl.c: In function 'xenFormatXLVnuma':
xenconfig/xen_xl.c:1264:5: error: format '%ld' expects argument of
type 'long int', but argument 3 has type 'size_t' [-Werror=format=]
  virBufferAsprintf(, "pnode=%ld", node);
  ^
xenconfig/xen_xl.c:1268:5: error: format '%ld' expects argument of
type 'long int', but argument 3 has type 'size_t' [-Werror=format=]
  virBufferAsprintf(, "size=%ld", nodeSize);
  ^
cc1: all warnings being treated as errors

I think this was introduced with 03d0959af "xenconfig: add domxml
conversions for xen-xl".


Thanks. I've pushed a patch to fix this

https://libvirt.org/git/?p=libvirt.git;a=commit;h=ce3b585bc4cec7db88da010651fe9fad15bf7173

Regards,
Jim


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [libvirt] Libvirt config converter can't handle file not ending with new line

[Jim Fehlig]

On 11/07/2017 08:54 AM, Wim ten Have wrote:

On Tue, 7 Nov 2017 12:20:05 +
Wei Liu <wei.l...@citrix.com> wrote:


On Mon, Nov 06, 2017 at 09:41:01PM -0700, Jim Fehlig wrote:

On 10/30/2017 06:17 AM, Wei Liu wrote:

Hi Jim

I discover a problem when using xen_xl converter. When the file in
question doesn't end with a new line, I get the following error:

error: configuration file syntax error: memory conf:53: expecting a value


I'm not able to reproduce this issue. The libvirt.git tree I tried was a bit
dated, but even after updating to latest master I can't reproduce.
   

After digging a bit (but haven't read libvirt code), it appears that the
file didn't end with a new line.


I tried several files without ending new lines, going both directions
(domxml-to-native and domxml-from-native), but didn't see the mentioned
error. Perhaps your config is revealing another bug which is being
improperly reported. Can you provide an example of the problematic config?
   


I tried to get the exact file that caused the problem but it is already
destroyed by osstest.

A similar file:

http://logs.test-lab.xenproject.org/osstest/logs/115436/test-amd64-amd64-libvirt-pair/debian.guest.osstest.cfg

If you hexdump -C it, you can see the last character is 0a. Remove it and
feed the file into the converter.
Wei.


   The phenonomem you point out is indeed weird.  And my first response
   is that this is a bug parsing the cfg input.  I did little explore and
   think that src/util/virconf.c (virConfParseLong(), virConfParseValue())
   should be reworked as pointed out in below context diffs.

<wtenhave@nina:140> git diff
diff --git a/src/util/virconf.c b/src/util/virconf.c
index 39c2bd917..bc8e57ec3 100644
--- a/src/util/virconf.c
+++ b/src/util/virconf.c
@@ -352,7 +352,7 @@ virConfParseLong(virConfParserCtxtPtr ctxt, long 
long *val)
 } else if (CUR == '+') {
 NEXT;
 }
-if ((ctxt->cur >= ctxt->end) || (!c_isdigit(CUR))) {
+if ((ctxt->cur > ctxt->end) || (!c_isdigit(CUR))) {
 virConfError(ctxt, VIR_ERR_CONF_SYNTAX, _("unterminated 
number"));
 return -1;
 }
@@ -456,7 +456,7 @@ virConfParseValue(virConfParserCtxtPtr ctxt)
 long long l = 0;

 SKIP_BLANKS;
-if (ctxt->cur >= ctxt->end) {
+if (ctxt->cur > ctxt->end) {
 virConfError(ctxt, VIR_ERR_CONF_SYNTAX, _("expecting a 
value"));
 return NULL;
 }

   I did not go beyond this yet.


Thanks Wim. I noticed Cole fixed a similar issue when parsing content from a 
file with commit 3cc2a9e0d4. But I think instead of replicating that fix in 
virConfReadString(), we should just set the end of content correctly in 
virConfParse(). I've sent a patch along those lines that fixes Wei's test case 
and doesn't regress Cole's test case


https://www.redhat.com/archives/libvir-list/2017-November/msg00286.html

Regards,
Jim


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] Libvirt config converter can't handle file not ending with new line

[Jim Fehlig]

On 10/30/2017 06:17 AM, Wei Liu wrote:

Hi Jim

I discover a problem when using xen_xl converter. When the file in
question doesn't end with a new line, I get the following error:

   error: configuration file syntax error: memory conf:53: expecting a value


I'm not able to reproduce this issue. The libvirt.git tree I tried was a bit 
dated, but even after updating to latest master I can't reproduce.



After digging a bit (but haven't read libvirt code), it appears that the
file didn't end with a new line.


I tried several files without ending new lines, going both directions 
(domxml-to-native and domxml-from-native), but didn't see the mentioned error. 
Perhaps your config is revealing another bug which is being improperly reported. 
Can you provide an example of the problematic config?


Regards,
Jim



___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] Libvirt xl to xml converter only picks up first occurrence of an option

[Jim Fehlig]

On 10/20/2017 08:46 AM, Wei Liu wrote:

Hi Jim


Hi Wei,

Sorry for the delay. Catching up on mail after some days off...


I discovered that libvirt's native config file to xml converter for
libxl only pick up the first occurrence of an option.

For example in a xl cfg file:

extra = "abc"
...
extra = "def"

xl will pick up "def" because that shows up later and takes precedence,
while the converter picks up "abc".

I think this is a bug in the converter and should be fixed.


Thanks for the report. I took a quick peek at libvirt's generic config parser 
and afaict it only searches for the first occurrence of a setting. The parser 
does support flags though, so we could add something like 
VIR_CONF_FLAG_{FIRST,LAST}_DUPLICATE. (Better name suggestions welcomed.)


I've opened a bug report against openSUSE Factory to track this

https://bugzilla.opensuse.org/show_bug.cgi?id=1065118

Regards,
Jim




Thanks
Wei.




___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] Renaming p9 to p9s in libxl idl

[Jim Fehlig]

On 08/11/2017 02:27 PM, Wei Liu wrote:

On Fri, Aug 11, 2017 at 12:37:04PM -0600, Jim Fehlig wrote:

On 08/11/2017 05:45 AM, Wei Liu wrote:

On Thu, Aug 10, 2017 at 03:42:24PM -0600, Jim Fehlig wrote:

On 08/08/2017 09:09 AM, Wei Liu wrote:

Ian and Stefano

Oleksandr discovered that the p9fs array in libxl_domain_config is name
p9 instead of p9s. This causes problem for his work to rework device
framework.

Given that p9fs was added only during last release and the only known
external toolstack libvirt can't possibility use that, maybe we can
rename p9 to p9s. Opinions?


ATM the libvirt libxl driver doesn't use it, but it could by supporting
libvirt's  device

http://libvirt.org/formatdomain.html#elementsFilesystems


I think that means all the parameters go directly to QEMU. Without
proper plumbing via libxl driver there won't be anything in the xenstore
hence it isn't useable by Xen guest, right?


I'm not sure why they have to go directly to QEMU. My naive thinking was to
map the  XML elements/attributes to libxl_device_p9 struct. E.g.
/domain/devices/filesystem/source@file would map to libxl_device_p9->path,
etc.



Right, that would require adding code somewhere in libvirt.git, right?

What I'm trying to figure out is if there could be code is libvirt that
uses the p9 array defined in libxl. It seems to me the answer is no.


Correct, not at this time. Perhaps in the near future :-).


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] Renaming p9 to p9s in libxl idl

[Jim Fehlig]

On 08/11/2017 05:45 AM, Wei Liu wrote:

On Thu, Aug 10, 2017 at 03:42:24PM -0600, Jim Fehlig wrote:

On 08/08/2017 09:09 AM, Wei Liu wrote:

Ian and Stefano

Oleksandr discovered that the p9fs array in libxl_domain_config is name
p9 instead of p9s. This causes problem for his work to rework device
framework.

Given that p9fs was added only during last release and the only known
external toolstack libvirt can't possibility use that, maybe we can
rename p9 to p9s. Opinions?


ATM the libvirt libxl driver doesn't use it, but it could by supporting
libvirt's  device

http://libvirt.org/formatdomain.html#elementsFilesystems


I think that means all the parameters go directly to QEMU. Without
proper plumbing via libxl driver there won't be anything in the xenstore
hence it isn't useable by Xen guest, right?


I'm not sure why they have to go directly to QEMU. My naive thinking was to map 
the  XML elements/attributes to libxl_device_p9 struct. E.g. 
/domain/devices/filesystem/source@file would map to libxl_device_p9->path, etc.


Regards,
Jim



___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] Renaming p9 to p9s in libxl idl

[Jim Fehlig]

On 08/08/2017 09:09 AM, Wei Liu wrote:

Ian and Stefano

Oleksandr discovered that the p9fs array in libxl_domain_config is name
p9 instead of p9s. This causes problem for his work to rework device
framework.

Given that p9fs was added only during last release and the only known
external toolstack libvirt can't possibility use that, maybe we can
rename p9 to p9s. Opinions?


ATM the libvirt libxl driver doesn't use it, but it could by supporting 
libvirt's  device


http://libvirt.org/formatdomain.html#elementsFilesystems

Regards,
Jim

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [libvirt test] 107696: regressions - FAIL

[Jim Fehlig]

On 05/02/2017 04:28 AM, Ian Jackson wrote:

Jim Fehlig writes ("Re: [Xen-devel] [libvirt test] 107696: regressions - FAIL"):

On 04/26/2017 02:34 PM, osstest service owner wrote:

flight 107696 libvirt real [real]
http://logs.test-lab.xenproject.org/osstest/logs/107696/

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 build-arm64-libvirt   5 libvirt-buildfail REGR. vs. 107640
 build-i386-libvirt5 libvirt-buildfail REGR. vs. 107640
 build-amd64-libvirt   5 libvirt-buildfail REGR. vs. 107640
 build-armhf-libvirt   5 libvirt-buildfail REGR. vs. 107640


I see the bisector has already fingered libvirt commit 02fb15fb,
which added a new submodule to libvirt.git. I'm an osstest noob, but
took a stab at fixing this with the attached patch.


Thanks.  Your patch is correct.  In fact, it is identical apart from
formatting to the patch I pushed to osstest pretest last week.


Ah, I didn't see your change. Sorry for the noise.


Unfortunately the persistent Windows heisenbugs mean that that osstest
change didn't get a push.  I will force push it.


Thanks!

Regards,
Jim


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [libvirt test] 107696: regressions - FAIL

[Jim Fehlig]

On 04/26/2017 02:34 PM, osstest service owner wrote:

flight 107696 libvirt real [real]
http://logs.test-lab.xenproject.org/osstest/logs/107696/

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 build-arm64-libvirt   5 libvirt-buildfail REGR. vs. 107640
 build-i386-libvirt5 libvirt-buildfail REGR. vs. 107640
 build-amd64-libvirt   5 libvirt-buildfail REGR. vs. 107640
 build-armhf-libvirt   5 libvirt-buildfail REGR. vs. 107640


I see the bisector has already fingered libvirt commit 02fb15fb, which added a 
new submodule to libvirt.git. I'm an osstest noob, but took a stab at fixing 
this with the attached patch.


Regards,
Jim

>From e1c9d55c046b6b826af95d3bb6cfaf63715f2923 Mon Sep 17 00:00:00 2001
From: Jim Fehlig <jfeh...@suse.com>
Date: Mon, 1 May 2017 16:45:10 -0600
Subject: [PATCH] osstest: add keycodemapdb to list of libvirt git submodules

libvirt commit 02fb15fb added a git submodule for keycode map,
doing away with the old, stale keymaps.csv copied from GTK-VNC.
This submodule needs to be handled by osstest when checking out
libvirt.

Signed-off-by: Jim Fehlig <jfeh...@suse.com>
---
 ts-libvirt-build | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ts-libvirt-build b/ts-libvirt-build
index 714402b..7bd5f80 100755
--- a/ts-libvirt-build
+++ b/ts-libvirt-build
@@ -26,7 +26,7 @@ tsreadconfig();
 selectbuildhost(\@ARGV);
 builddirsprops();
 
-our %submodmap = qw(gnulib gnulib);
+our %submodmap = qw(gnulib gnulib keycodemapdb keycodemapdb);
 our $submodules;
 
 sub libvirtd_init ();
-- 
2.11.0

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [libvirt test] 106952: regressions - FAIL

[Jim Fehlig]

On 03/28/2017 02:09 AM, osstest service owner wrote:

flight 106952 libvirt real [real]
http://logs.test-lab.xenproject.org/osstest/logs/106952/

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 build-i386-libvirt5 libvirt-buildfail REGR. vs. 106829
 build-armhf-libvirt   5 libvirt-buildfail REGR. vs. 106829


Fixed by

http://libvirt.org/git/?p=libvirt.git;a=commit;h=efb446e1b0ee51671063089c6988a7977098a800

Regards,
Jim

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [libvirt test] 106883: regressions - FAIL

[Jim Fehlig]

On 03/24/2017 12:17 PM, osstest service owner wrote:

flight 106883 libvirt real [real]
http://logs.test-lab.xenproject.org/osstest/logs/106883/

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 build-armhf-libvirt   5 libvirt-buildfail REGR. vs. 106829


Cedric,

It looks like one of your recent libvirt changes triggers this build failure

util/virnetdevip.c: In function 'virNetDevIPCheckIPv6ForwardingCallback':
util/virnetdevip.c:560:17: error: cast increases required alignment of target 
type [-Werror=cast-align]

 for (rta = RTM_RTA(rtmsg); RTA_OK(rta, len); rta = RTA_NEXT(rta, len)) {

Regards,
Jim

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] libxl/xl: add support for Xen 9pfs

[Jim Fehlig]

On 03/23/2017 05:36 PM, Stefano Stabellini wrote:

Add functions to libxl to setup a Xen 9pfs frontend/backend connection.
Add support to xl to parse a xen_9pfs option in the VM config file, in
the following format:

xen_9pfs=["tag=share_dir,security_model=none,path=/path/share_dir"]

where tag identifies the 9p share and it is required to mount it on the
guest side, path is the path of the filesystem to share and the only
security_model supported is "none" which means that files are stored
using the same credentials as they are created on the guest (no user
ownership squash or remap).


FYI, similar libvirt config:

  


  

So security_model == accessmode. The docs for accessmode:

-
The filesystem block has an optional attribute accessmode which specifies the 
security mode for accessing the source (since 0.8.5). Currently this only works 
with type='mount' for the QEMU/KVM driver. The possible values are:


passthrough
The source is accessed with the permissions of the user inside the guest. This 
is the default accessmode if one is not specified.


mapped
The source is accessed with the permissions of the hypervisor (QEMU process).

squash
Similar to 'passthrough', the exception is that failure of privileged operations 
like 'chown' are ignored. This makes a passthrough-like mode usable for people 
who run the hypervisor as non-root.

-

Regards,
Jim


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [libvirt] [PATCH 2/2] libxl: fix dom0 maximum memory setting

[Jim Fehlig]

Joao Martins wrote:
> On 02/08/2017 04:06 PM, Jim Fehlig wrote:
>> Joao Martins wrote:
>>> On 02/02/2017 10:31 PM, Jim Fehlig wrote:
>>>> When the libxl driver is initialized, it creates a virDomainDef
>>>> object for dom0 and adds it to the list of domains. Total memory
>>>> for dom0 was being set from the max_memkb field of libxl_dominfo
>>>> struct retrieved from libxl, but this field can be set to
>>>> LIBXL_MEMKB_DEFAULT (~0ULL) if dom0 maximum memory has not been
>>>> explicitly set by the user.
>>>>
>>>> This patch adds some simple parsing of the Xen commandline,
>>>> looking for a dom0_mem parameter that also specifies a 'max' value.
>>>> If not specified, dom0 maximum memory is effectively all physical
>>>> host memory.
>>>>
>>>> Signed-off-by: Jim Fehlig <jfeh...@suse.com>
>>>> ---
>>>>  src/libxl/libxl_conf.c   | 75 
>>>> 
>>>>  src/libxl/libxl_conf.h   |  3 ++
>>>>  src/libxl/libxl_driver.c |  2 +-
>>>>  3 files changed, 79 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
>>>> index b5186f2..bfe0e92 100644
>>>> --- a/src/libxl/libxl_conf.c
>>>> +++ b/src/libxl/libxl_conf.c
>>>> @@ -34,6 +34,7 @@
>>>>  #include "internal.h"
>>>>  #include "virlog.h"
>>>>  #include "virerror.h"
>>>> +#include "c-ctype.h"
>>>>  #include "datatypes.h"
>>>>  #include "virconf.h"
>>>>  #include "virfile.h"
>>>> @@ -1530,6 +1531,80 @@ int libxlDriverConfigLoadFile(libxlDriverConfigPtr 
>>>> cfg,
>>>>  
>>>>  }
>>>>  
>>>> +/*
>>>> + * dom0's maximum memory can be controled by the user with the 'dom0_mem' 
>>>> Xen
>>>> + * command line parameter. E.g. to set dom0's initial memory to 4G and max
>>>> + * memory to 8G: dom0_mem=4G,max:8G
>>>> + *
>>>> + * If not constrained by the user, dom0 can effectively use all host 
>>>> memory.
>>>> + * This function returns the configured maximum memory for dom0 in 
>>>> kilobytes,
>>>> + * either the user-specified value or total physical memory as a default.
>>>> + */
>>>> +unsigned long long
>>>> +libxlDriverGetDom0MaxmemConf(libxlDriverConfigPtr cfg)
>>>> +{
>>>> +char **cmd_tokens = NULL;
>>>> +char **mem_tokens = NULL;
>>>> +size_t i;
>>>> +size_t j;
>>>> +unsigned long long ret;
>>>> +libxl_physinfo physinfo;
>>>> +
>>>> +if (cfg->verInfo->commandline == NULL ||
>>>> +!(cmd_tokens = virStringSplit(cfg->verInfo->commandline, " ", 0)))
>>>> +goto physmem;
>>>> +
>>>> +for (i = 0; cmd_tokens[i] != NULL; i++) {
>>>> +if (!STRPREFIX(cmd_tokens[i], "dom0_mem="))
>>>> +continue;
>>>> +
>>>> +if (!(mem_tokens = virStringSplit(cmd_tokens[i], ",", 0)))
>>>> +break;
>>>> +for (j = 0; mem_tokens[j] != NULL; j++) {
>>>> +if (STRPREFIX(mem_tokens[j], "max:")) {
>>>> +char *p = mem_tokens[j] + 4;
>>>> +unsigned long long multiplier = 1;
>>>> +
>>>> +while (c_isdigit(*p))
>>>> +p++;
>>>> +if (virStrToLong_ull(mem_tokens[j] + 4, , 10, ) < 0)
>>>> +break;
>>>> +if (*p) {
>>>> +switch (*p) {
>>>> +case 'k':
>>>> +case 'K':
>>>> +multiplier = 1024;
>>>> +break;
>>>> +case 'm':
>>>> +case 'M':
>>>> +multiplier = 1024 * 1024;
>>>> +break;
>>>> +case 'g':
>>>> +case 'G':
>>>> +multiplier = 1024 * 1024 * 1024;
>>>> +break;
>>>> +

Re: [Xen-devel] [libvirt] [PATCH 2/2] libxl: fix dom0 maximum memory setting

[Jim Fehlig]

Joao Martins wrote:
> On 02/02/2017 10:31 PM, Jim Fehlig wrote:
>> When the libxl driver is initialized, it creates a virDomainDef
>> object for dom0 and adds it to the list of domains. Total memory
>> for dom0 was being set from the max_memkb field of libxl_dominfo
>> struct retrieved from libxl, but this field can be set to
>> LIBXL_MEMKB_DEFAULT (~0ULL) if dom0 maximum memory has not been
>> explicitly set by the user.
>>
>> This patch adds some simple parsing of the Xen commandline,
>> looking for a dom0_mem parameter that also specifies a 'max' value.
>> If not specified, dom0 maximum memory is effectively all physical
>> host memory.
>>
>> Signed-off-by: Jim Fehlig <jfeh...@suse.com>
>> ---
>>  src/libxl/libxl_conf.c   | 75 
>> 
>>  src/libxl/libxl_conf.h   |  3 ++
>>  src/libxl/libxl_driver.c |  2 +-
>>  3 files changed, 79 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
>> index b5186f2..bfe0e92 100644
>> --- a/src/libxl/libxl_conf.c
>> +++ b/src/libxl/libxl_conf.c
>> @@ -34,6 +34,7 @@
>>  #include "internal.h"
>>  #include "virlog.h"
>>  #include "virerror.h"
>> +#include "c-ctype.h"
>>  #include "datatypes.h"
>>  #include "virconf.h"
>>  #include "virfile.h"
>> @@ -1530,6 +1531,80 @@ int libxlDriverConfigLoadFile(libxlDriverConfigPtr 
>> cfg,
>>  
>>  }
>>  
>> +/*
>> + * dom0's maximum memory can be controled by the user with the 'dom0_mem' 
>> Xen
>> + * command line parameter. E.g. to set dom0's initial memory to 4G and max
>> + * memory to 8G: dom0_mem=4G,max:8G
>> + *
>> + * If not constrained by the user, dom0 can effectively use all host memory.
>> + * This function returns the configured maximum memory for dom0 in 
>> kilobytes,
>> + * either the user-specified value or total physical memory as a default.
>> + */
>> +unsigned long long
>> +libxlDriverGetDom0MaxmemConf(libxlDriverConfigPtr cfg)
>> +{
>> +char **cmd_tokens = NULL;
>> +char **mem_tokens = NULL;
>> +size_t i;
>> +size_t j;
>> +unsigned long long ret;
>> +libxl_physinfo physinfo;
>> +
>> +if (cfg->verInfo->commandline == NULL ||
>> +!(cmd_tokens = virStringSplit(cfg->verInfo->commandline, " ", 0)))
>> +goto physmem;
>> +
>> +for (i = 0; cmd_tokens[i] != NULL; i++) {
>> +if (!STRPREFIX(cmd_tokens[i], "dom0_mem="))
>> +continue;
>> +
>> +if (!(mem_tokens = virStringSplit(cmd_tokens[i], ",", 0)))
>> +break;
>> +for (j = 0; mem_tokens[j] != NULL; j++) {
>> +if (STRPREFIX(mem_tokens[j], "max:")) {
>> +char *p = mem_tokens[j] + 4;
>> +unsigned long long multiplier = 1;
>> +
>> +while (c_isdigit(*p))
>> +p++;
>> +if (virStrToLong_ull(mem_tokens[j] + 4, , 10, ) < 0)
>> +break;
>> +if (*p) {
>> +switch (*p) {
>> +case 'k':
>> +case 'K':
>> +multiplier = 1024;
>> +break;
>> +case 'm':
>> +case 'M':
>> +multiplier = 1024 * 1024;
>> +break;
>> +case 'g':
>> +case 'G':
>> +multiplier = 1024 * 1024 * 1024;
>> +break;
>> +}
>> +}
>> +ret = (ret * multiplier) / 1024;
>> +goto cleanup;
>> +}
>> +}
>> +}
>> +
>> + physmem:
>> +/* No 'max' specified in dom0_mem, so dom0 can use all physical memory 
>> */
>> +libxl_physinfo_init();
>> +libxl_get_physinfo(cfg->ctx, );
> Despite being an unlikely event, libxl_get_physinfo can fail here - I think 
> you
> need to check the return value here.

Bummer. I was hoping this function could always return a sane value for dom0 max
memory. But I'm not really sure what that would be if libxl_get_physinfo failed.

> 
>> +ret = (physinfo.total_pages * cfg->verInfo->pagesize) / 1024;
>> +libxl_physinfo_dispose();
>&g

[Xen-devel] [PATCH 2/2] libxl: fix dom0 maximum memory setting

[Jim Fehlig]

When the libxl driver is initialized, it creates a virDomainDef
object for dom0 and adds it to the list of domains. Total memory
for dom0 was being set from the max_memkb field of libxl_dominfo
struct retrieved from libxl, but this field can be set to
LIBXL_MEMKB_DEFAULT (~0ULL) if dom0 maximum memory has not been
explicitly set by the user.

This patch adds some simple parsing of the Xen commandline,
looking for a dom0_mem parameter that also specifies a 'max' value.
If not specified, dom0 maximum memory is effectively all physical
host memory.

Signed-off-by: Jim Fehlig <jfeh...@suse.com>
---
 src/libxl/libxl_conf.c   | 75 
 src/libxl/libxl_conf.h   |  3 ++
 src/libxl/libxl_driver.c |  2 +-
 3 files changed, 79 insertions(+), 1 deletion(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index b5186f2..bfe0e92 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -34,6 +34,7 @@
 #include "internal.h"
 #include "virlog.h"
 #include "virerror.h"
+#include "c-ctype.h"
 #include "datatypes.h"
 #include "virconf.h"
 #include "virfile.h"
@@ -1530,6 +1531,80 @@ int libxlDriverConfigLoadFile(libxlDriverConfigPtr cfg,
 
 }
 
+/*
+ * dom0's maximum memory can be controled by the user with the 'dom0_mem' Xen
+ * command line parameter. E.g. to set dom0's initial memory to 4G and max
+ * memory to 8G: dom0_mem=4G,max:8G
+ *
+ * If not constrained by the user, dom0 can effectively use all host memory.
+ * This function returns the configured maximum memory for dom0 in kilobytes,
+ * either the user-specified value or total physical memory as a default.
+ */
+unsigned long long
+libxlDriverGetDom0MaxmemConf(libxlDriverConfigPtr cfg)
+{
+char **cmd_tokens = NULL;
+char **mem_tokens = NULL;
+size_t i;
+size_t j;
+unsigned long long ret;
+libxl_physinfo physinfo;
+
+if (cfg->verInfo->commandline == NULL ||
+!(cmd_tokens = virStringSplit(cfg->verInfo->commandline, " ", 0)))
+goto physmem;
+
+for (i = 0; cmd_tokens[i] != NULL; i++) {
+if (!STRPREFIX(cmd_tokens[i], "dom0_mem="))
+continue;
+
+if (!(mem_tokens = virStringSplit(cmd_tokens[i], ",", 0)))
+break;
+for (j = 0; mem_tokens[j] != NULL; j++) {
+if (STRPREFIX(mem_tokens[j], "max:")) {
+char *p = mem_tokens[j] + 4;
+unsigned long long multiplier = 1;
+
+while (c_isdigit(*p))
+p++;
+if (virStrToLong_ull(mem_tokens[j] + 4, , 10, ) < 0)
+break;
+if (*p) {
+switch (*p) {
+case 'k':
+case 'K':
+multiplier = 1024;
+break;
+case 'm':
+case 'M':
+multiplier = 1024 * 1024;
+break;
+case 'g':
+case 'G':
+multiplier = 1024 * 1024 * 1024;
+break;
+}
+}
+ret = (ret * multiplier) / 1024;
+goto cleanup;
+}
+}
+}
+
+ physmem:
+/* No 'max' specified in dom0_mem, so dom0 can use all physical memory */
+libxl_physinfo_init();
+libxl_get_physinfo(cfg->ctx, );
+ret = (physinfo.total_pages * cfg->verInfo->pagesize) / 1024;
+libxl_physinfo_dispose();
+
+ cleanup:
+virStringListFree(cmd_tokens);
+virStringListFree(mem_tokens);
+return ret;
+}
+
+
 #ifdef LIBXL_HAVE_DEVICE_CHANNEL
 static int
 libxlPrepareChannel(virDomainChrDefPtr channel,
diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h
index 69d7885..c4ddbfe 100644
--- a/src/libxl/libxl_conf.h
+++ b/src/libxl/libxl_conf.h
@@ -173,6 +173,9 @@ libxlDriverNodeGetInfo(libxlDriverPrivatePtr driver,
 int libxlDriverConfigLoadFile(libxlDriverConfigPtr cfg,
   const char *filename);
 
+unsigned long long
+libxlDriverGetDom0MaxmemConf(libxlDriverConfigPtr cfg);
+
 int
 libxlMakeDisk(virDomainDiskDefPtr l_dev, libxl_device_disk *x_dev);
 int
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 921cc93..e54b3b7 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -615,7 +615,7 @@ libxlAddDom0(libxlDriverPrivatePtr driver)
 if (virDomainDefSetVcpus(vm->def, d_info.vcpu_online) < 0)
 goto cleanup;
 vm->def->mem.cur_balloon = d_info.current_memkb;
-virDomainDefSetMemoryTotal(vm->def, d_info.max_memkb);
+virDomainDefSetMemoryTotal(vm->def, libxlDriverGetDom0MaxmemConf(cfg));
 
 ret = 0;
 
-- 
2.9.2


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [PATCH 1/2] libxl: fix reporting of maximum memory

[Jim Fehlig]

The libxl driver reports different values of maximum memory depending
on state of a domain. If inactive, maximum memory value is reported
correctly. When active, maximum memory is derived from max_pages value
returned by the XEN_SYSCTL_getdomaininfolist sysctl operation. But
max_pages can be changed by toolstacks and does not necessarily
represent the maximum memory a domain can use during its active
lifetime.

A better location for determining a domain's maximum memory is the
/local/domain//memory/static-max node in xenstore. This value
is set from the libxl_domain_build_info.max_memkb field when creating
the domain. Currently it cannot be changed nor can its value be
exceeded by a balloon operation. From libvirt's perspective, always
reporting maximum memory with virDomainDefGetMemoryTotal() will produce
the same results as reading the static-max node in xenstore.

Signed-off-by: Jim Fehlig <jfeh...@suse.com>
---

As an alternative, libvirt could call libxl_retrieve_domain_configuration
and use the max_memkb field embedded in the libxl_domain_config object.

 src/libxl/libxl_driver.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 3a69720..921cc93 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -1640,10 +1640,10 @@ libxlDomainGetInfo(virDomainPtr dom, virDomainInfoPtr 
info)
 if (virDomainGetInfoEnsureACL(dom->conn, vm->def) < 0)
 goto cleanup;
 
+info->maxMem = virDomainDefGetMemoryTotal(vm->def);
 if (!virDomainObjIsActive(vm)) {
 info->cpuTime = 0;
 info->memory = vm->def->mem.cur_balloon;
-info->maxMem = virDomainDefGetMemoryTotal(vm->def);
 } else {
 libxl_dominfo_init(_info);
 
@@ -1655,7 +1655,6 @@ libxlDomainGetInfo(virDomainPtr dom, virDomainInfoPtr 
info)
 }
 info->cpuTime = d_info.cpu_time;
 info->memory = d_info.current_memkb;
-info->maxMem = d_info.max_memkb;
 
 libxl_dominfo_dispose(_info);
 }
@@ -5172,7 +5171,7 @@ libxlDomainMemoryStats(virDomainPtr dom,
 goto endjob;
 }
 mem = d_info.current_memkb;
-maxmem = d_info.max_memkb;
+maxmem = virDomainDefGetMemoryTotal(vm->def);
 
 LIBXL_SET_MEMSTAT(VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON, mem);
 LIBXL_SET_MEMSTAT(VIR_DOMAIN_MEMORY_STAT_AVAILABLE, maxmem);
-- 
2.9.2


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [PATCH 0/2] libxl: a few domain maximum memory fixes

[Jim Fehlig]

Patch 1 fixes reporting of domain maximum memory for running domains.
When creating a virDomainDef object to represent dom0, max memory was
not set correctly, which is fixed by patch2.

Jim Fehlig (2):
  libxl: fix reporting of maximum memory
  libxl: fix dom0 maximum memory setting

 src/libxl/libxl_conf.c   | 75 
 src/libxl/libxl_conf.h   |  3 ++
 src/libxl/libxl_driver.c |  7 ++---
 3 files changed, 81 insertions(+), 4 deletions(-)

-- 
2.9.2


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] libxl: fix dom0 autoballooning with Xen 4.8

[Jim Fehlig]

On 02/02/2017 04:42 AM, Wei Liu wrote:

I saw this mail but didn't realise it required my input, sorry.


I suppose it didn't and I was shamelessly fishing for a review - so you have my 
apologies :-). But the patch does fix an annoying, easily encountered bug which 
I'm eager to see resolved in the next libvirt release.




On Wed, Feb 01, 2017 at 04:00:54PM -0700, Jim Fehlig wrote:

Jim Fehlig wrote:

xen.git commit 57f8b13c changed several of the libxl memory
get/set functions to take 64 bit parameters. The libvirt
libxl driver still uses uint32_t variables for these various
parameters, which is particularly problematic for the
libxl_set_memory_target() function.

When dom0 autoballooning is enabled, libvirt (like xl) determines
the memory needed to start a domain and the memory available. If
memory available is less than memory needed, dom0 is ballooned
down by passing a negative value to libxl_set_memory_target()
'target_memkb' parameter. Prior to xen.git commit 57f8b13c,
'target_memkb' was an int32_t. Subtracting a larger uint32 from
a smaller uint32 and assigning it to int32 resulted in a negative
number. After commit 57f8b13c, the same subtraction is widened
to a int64, resulting in a large positive number. The simple
fix taken by this patch is to assign the difference of the
uint32 values to a temporary int32 variable, which is then
passed to 'target_memkb' parameter of libxl_set_memory_target().

Note that it is undesirable to change libvirt to use 64 bit
variables since it requires setting LIBXL_API_VERSION to 0x040800.
Currently libvirt supports LIBXL_API_VERSION >= 0x040400,
essentially Xen >= 4.4.


Any comments on this patch? xen-devel is already in cc, but I'll add Wei (one of
the Xen tools maintainers) and Juergen (author of commit 57f8b13c) for some
additional exposure :-).

Currently, "out-of-the-box" Xen >= 4.8 + libvirt setup (dom0 autoballooning
enabled) is broken without this fix.

Regards,
Jim



Signed-off-by: Jim Fehlig <jfeh...@suse.com>
---
 src/libxl/libxl_domain.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index a5314b0..ed73cd2 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -909,6 +909,7 @@ libxlDomainFreeMem(libxl_ctx *ctx, libxl_domain_config 
*d_config)
 {
 uint32_t needed_mem;
 uint32_t free_mem;
+int32_t target_mem;
 int tries = 3;
 int wait_secs = 10;

@@ -922,7 +923,8 @@ libxlDomainFreeMem(libxl_ctx *ctx, libxl_domain_config 
*d_config)
 if (free_mem >= needed_mem)
 return 0;

-if (libxl_set_memory_target(ctx, 0, free_mem - needed_mem,
+target_mem = free_mem - needed_mem;
+if (libxl_set_memory_target(ctx, 0, target_mem,


This should do the trick.


Thanks. There are other ways to skin this cat and if folks prefer one of those I 
can push a followup. But for now, so that we have a fix, I'll push this variant.


Regards,
Jim


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] libxl: fix dom0 autoballooning with Xen 4.8

[Jim Fehlig]

Jim Fehlig wrote:
> xen.git commit 57f8b13c changed several of the libxl memory
> get/set functions to take 64 bit parameters. The libvirt
> libxl driver still uses uint32_t variables for these various
> parameters, which is particularly problematic for the
> libxl_set_memory_target() function.
> 
> When dom0 autoballooning is enabled, libvirt (like xl) determines
> the memory needed to start a domain and the memory available. If
> memory available is less than memory needed, dom0 is ballooned
> down by passing a negative value to libxl_set_memory_target()
> 'target_memkb' parameter. Prior to xen.git commit 57f8b13c,
> 'target_memkb' was an int32_t. Subtracting a larger uint32 from
> a smaller uint32 and assigning it to int32 resulted in a negative
> number. After commit 57f8b13c, the same subtraction is widened
> to a int64, resulting in a large positive number. The simple
> fix taken by this patch is to assign the difference of the
> uint32 values to a temporary int32 variable, which is then
> passed to 'target_memkb' parameter of libxl_set_memory_target().
> 
> Note that it is undesirable to change libvirt to use 64 bit
> variables since it requires setting LIBXL_API_VERSION to 0x040800.
> Currently libvirt supports LIBXL_API_VERSION >= 0x040400,
> essentially Xen >= 4.4.

Any comments on this patch? xen-devel is already in cc, but I'll add Wei (one of
the Xen tools maintainers) and Juergen (author of commit 57f8b13c) for some
additional exposure :-).

Currently, "out-of-the-box" Xen >= 4.8 + libvirt setup (dom0 autoballooning
enabled) is broken without this fix.

Regards,
Jim

> 
> Signed-off-by: Jim Fehlig <jfeh...@suse.com>
> ---
>  src/libxl/libxl_domain.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
> index a5314b0..ed73cd2 100644
> --- a/src/libxl/libxl_domain.c
> +++ b/src/libxl/libxl_domain.c
> @@ -909,6 +909,7 @@ libxlDomainFreeMem(libxl_ctx *ctx, libxl_domain_config 
> *d_config)
>  {
>  uint32_t needed_mem;
>  uint32_t free_mem;
> +int32_t target_mem;
>  int tries = 3;
>  int wait_secs = 10;
>  
> @@ -922,7 +923,8 @@ libxlDomainFreeMem(libxl_ctx *ctx, libxl_domain_config 
> *d_config)
>  if (free_mem >= needed_mem)
>  return 0;
>  
> -if (libxl_set_memory_target(ctx, 0, free_mem - needed_mem,
> +target_mem = free_mem - needed_mem;
> +if (libxl_set_memory_target(ctx, 0, target_mem,
>  /* relative */ 1, 0) < 0)
>  goto error;
>  


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] memory hotplug for domUs

[Jim Fehlig]

On 01/20/2017 09:06 AM, Juergen Gross wrote:

On 20/01/17 16:54, Ian Jackson wrote:

Juergen Gross writes ("memory hotplug for domUs"):

We first thought to enhance "xl mem-set", but after some more thinking
about it I'd rather add a new xl command, e.g. "mem-add" (we could later
even add "mem-remove" to support memory unplug).


Why ?  Why would xl mem-set not automatically do the right thing ?


How would you specify the numa node to add the memory to?


And the host numa node providing the memory?


Especially for removal of memory ballooning and hotplug are _very_
different.


I agree. libvirt already models ballooning and hot(un)plug differently.

For ballooning, typical VM configuration is

  16777216
  2097152
  

'virsh setmaxmem' and 'virsh setmem' allow adjusting runtime and/or persistent 
configuration.


For hot(un)plug (currently only supported by the qemu driver) typical 
configuration is


  16777216
  

  
0-1
  
  
1048576
0
  

...


which describes a VM with 16 dimm slots with total capacity of 16G. One slot is 
filled with a 1G memory module whose memory must come from host numa nodes 0 or 
1, and is provided to numa node 0 in the VM.


'virsh attach-device' and virsh detach-device' allow adding or removing memory 
modules from the dimms. AIUI, under the covers qemu uses ACPI memory hotplug.





This problem with lack of support (and indeed requirementse to do
hotplug things) is a detail that in the long term should not be a
problem, and in the meantime can be dealt with by a force option.


I think it would be clearer with new commands for hotplugging. I
don't feel very strong about this, so in case everyone else is fine
with handling everything via mem-set I won't object.


If ACPI memory hotplug is indeed the goal, then I agree new commands should be 
used since it is quite different than ballooning.


Regards,
Jim


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2] docs: clarify xl mem-max semantics

[Jim Fehlig]

On 01/23/2017 02:49 AM, Juergen Gross wrote:

The information given in the xl man page for the mem-max command is
rather brief. Expand it in order to let the reader understand what it
is really doing.

As the related libxl function libxl_domain_setmaxmem() isn't much
clearer add a comment to it explaining the desired semantics.

Signed-off-by: Juergen Gross <jgr...@suse.com>
---
V2: rewording as suggested by Jim Fehlig and Ian Jackson
---
 docs/man/xl.pod.1.in | 10 ++
 tools/libxl/libxl.c  |  6 ++
 2 files changed, 16 insertions(+)

diff --git a/docs/man/xl.pod.1.in b/docs/man/xl.pod.1.in
index 09c1faa..7caed08 100644
--- a/docs/man/xl.pod.1.in
+++ b/docs/man/xl.pod.1.in
@@ -401,6 +401,16 @@ for bytes.
 The mem-max value may not correspond to the actual memory used in the
 domain, as it may balloon down its memory to give more back to the OS.

+The value given just sets the memory amount the domain is allowed to allocate
+in the hypervisor. It can't be set lower than the current reservation, but
+it is allowed to be higher than the configured maximum memory size of the
+domain (B parameter in the domain's configuration). Using B
+to set the maximum memory above the initial B value will not allow the
+additional memory to be used via B. The initial B value is
+still used as an upper limit for B.
+
+The domain is not receiving any signal regarding the changed memory limit.
+
 =item B I I

 Set the domain's used memory using the balloon driver; append 't' for
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 0622311..ed59510 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -4018,6 +4018,12 @@ out:

 
/**/

+/*
+ * Set the maximum memory size of the domain in the hypervisor. There is no
+ * change of the current memory size involved. The allowed memory size can


s/allowed/specified/ ? Or perhaps "The value of max_memkb can even..." ? Sorry I 
didn't look closely at this hunk in V1.



+ * even be above the configured maxmem size of the domain, but the related
+ * Xenstore entry memory/static-max isn't modified!
+ */
 int libxl_domain_setmaxmem(libxl_ctx *ctx, uint32_t domid, uint64_t max_memkb)
 {
 GC_INIT(ctx);


I have a related question for you (or Ian, Wei, ...): What does max_pages (as 
seen with 'q' debug key') represent? How is it related to memory/static-max in 
xenstore, if at all?


When a domain is running, libvirt reports maximum memory through max_memkb field 
of libxl_dominfo struct. max_memkb is filled with 
PAGE_TO_MEMKB(xcinfo->max_pages). AFAICT, xcinfo->max_pages contains the same 
value as max_pages from 'q' debug key, but neither of those represent the maxmem 
value used at domain creation. E.g.


# cat test.xl | grep mem
maxmem = 8192
memory = 2048
# xl create test.xl
# xl debug-key q && xl dmesg | grep max_pages
(XEN) nr_pages=524205 xenheap_pages=6 shared_pages=0 paged_pages=0 
dirty_cpus={20,42} max_pages=524544


On this system, 524544*4096=2049M which is closer to 'memory' than 'maxmem'.

I have a patch which fixes this in libvirt, but I'm having a hard time 
articulating why the current use of max_pages is incorrect :-).


TIA for any comments you might have on that.

Regards,
Jim


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] docs: clarify xl mem-max semantics

[Jim Fehlig]

On 01/20/2017 02:54 AM, Juergen Gross wrote:

The information given in the xl man page for the mem-max command is
rather brief. Expand it in order to let the reader understand what it
is really doing.

As the related libxl function libxl_domain_setmaxmem() isn't much
clearer add a comment to it explaining the desired semantics.

Signed-off-by: Juergen Gross 
---
 docs/man/xl.pod.1.in | 10 ++
 tools/libxl/libxl.c  |  6 ++
 2 files changed, 16 insertions(+)

diff --git a/docs/man/xl.pod.1.in b/docs/man/xl.pod.1.in
index 09c1faa..62307e8 100644
--- a/docs/man/xl.pod.1.in
+++ b/docs/man/xl.pod.1.in
@@ -401,6 +401,16 @@ for bytes.
 The mem-max value may not correspond to the actual memory used in the
 domain, as it may balloon down its memory to give more back to the OS.

+The value given just sets the memory amount the domain is allowed to allocate
+in the hypervisor. Thus it can't be lower than the current reservation, but
+it is allowed to be higher than the configured maximum memory size of the
+domain (B parameter in the domain's configuration). Setting the
+allowed memory size via B above the B size won't let use
+this value to be used for B, as B will still use
+B as an upper limit.


I find the last sentence a bit awkward. Might I suggest

Using B to set the maximum memory above the initial B value 
will not allow the additional memory to be used via B. The initial 
B value is still used as an upper limit for B.


Regards,
Jim


+
+The domain is not receiving any signal regarding the changed memory limit.
+
 =item B I I

 Set the domain's used memory using the balloon driver; append 't' for
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 0622311..ed59510 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -4018,6 +4018,12 @@ out:

 
/**/

+/*
+ * Set the maximum memory size of the domain in the hypervisor. There is no
+ * change of the current memory size involved. The allowed memory size can
+ * even be above the configured maxmem size of the domain, but the related
+ * Xenstore entry memory/static-max isn't modified!
+ */
 int libxl_domain_setmaxmem(libxl_ctx *ctx, uint32_t domid, uint64_t max_memkb)
 {
 GC_INIT(ctx);




___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [PATCH] libxl: fix dom0 autoballooning with Xen 4.8

[Jim Fehlig]

xen.git commit 57f8b13c changed several of the libxl memory
get/set functions to take 64 bit parameters. The libvirt
libxl driver still uses uint32_t variables for these various
parameters, which is particularly problematic for the
libxl_set_memory_target() function.

When dom0 autoballooning is enabled, libvirt (like xl) determines
the memory needed to start a domain and the memory available. If
memory available is less than memory needed, dom0 is ballooned
down by passing a negative value to libxl_set_memory_target()
'target_memkb' parameter. Prior to xen.git commit 57f8b13c,
'target_memkb' was an int32_t. Subtracting a larger uint32 from
a smaller uint32 and assigning it to int32 resulted in a negative
number. After commit 57f8b13c, the same subtraction is widened
to a int64, resulting in a large positive number. The simple
fix taken by this patch is to assign the difference of the
uint32 values to a temporary int32 variable, which is then
passed to 'target_memkb' parameter of libxl_set_memory_target().

Note that it is undesirable to change libvirt to use 64 bit
variables since it requires setting LIBXL_API_VERSION to 0x040800.
Currently libvirt supports LIBXL_API_VERSION >= 0x040400,
essentially Xen >= 4.4.

Signed-off-by: Jim Fehlig <jfeh...@suse.com>
---
 src/libxl/libxl_domain.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index a5314b0..ed73cd2 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -909,6 +909,7 @@ libxlDomainFreeMem(libxl_ctx *ctx, libxl_domain_config 
*d_config)
 {
 uint32_t needed_mem;
 uint32_t free_mem;
+int32_t target_mem;
 int tries = 3;
 int wait_secs = 10;
 
@@ -922,7 +923,8 @@ libxlDomainFreeMem(libxl_ctx *ctx, libxl_domain_config 
*d_config)
 if (free_mem >= needed_mem)
 return 0;
 
-if (libxl_set_memory_target(ctx, 0, free_mem - needed_mem,
+target_mem = free_mem - needed_mem;
+if (libxl_set_memory_target(ctx, 0, target_mem,
 /* relative */ 1, 0) < 0)
 goto error;
 
-- 
2.9.2


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [libvirt] Question about hypervisor that are not tristate

[Jim Fehlig]

On 01/11/2017 10:55 AM, Daniel P. Berrange wrote:

On Wed, Jan 11, 2017 at 10:49:48AM -0700, Jim Fehlig wrote:

On 01/06/2017 05:31 PM, Jim Fehlig wrote:

Adding xen-devel for a question below...


Happy new year!

Nearly a year ago I reported an issue with the  hypervisor feature on Xen
[1] and am now seeing a similar issue with the  feature. Setting the
default value of pae changed between xend and libxl. When not specified, xend
would enable pae for HVM domains. Clients such as xm and the old libvirt driver
did not have to explicitly enable it. In libxl, the pae field within
libxl_domain_build_info is initialized to 0. Clients must enable pae, and indeed
xl will do so if pae=1 is not specified in the xl.cfg.

The xend behavior prevents libvirt from disabling pae, whereas the libxl behvior
causes a guest ABI change (config that worked with libvirt+xend doesn't with
libvirt+libxl). The libxl behavior also forces management software (e.g.
OpenStack nova) to add  where it wasn't needed before.

To solve this problem for , it was changed it to a tristate [2], allowing
it to be turned off with explicit , and on if not specified or
 or . Should  (and the remaining hypervisor features
that are not tristate) be converted to tristate similar to ? Alternatively,
I could simply set pae=1 for all HVM domains in the libxl driver. Like the old
libvirt+xend behavior it couldn't be turned off, but I don't think there is a
practical use-case to do so. At least no one has complained over all the years
of libvirt+xend use.


Xen folks, what is your opinion of always enabling pae for HVM domains in
the libvirt libxl driver? Is there a need these days to disable it?

Jan had mentioned that some old, buggy guest OS's (Win 9x) might need it
disabled, and perhaps some cases where it may be desirable to suppress a
guest OS entering 64-bit mode. But in practice do you think it is necessary
to expose this knob to users?


If the guest arch is declared as x86_64, then you should unconditionally
force 'pae' to be present, otherwise you'd be giving the guest an i686
environment.


Right.


The app should request arch == i686 explicitly if they
want an i686 environment. Allowing pae to be toggled for i686 guests
is ok, since that indicates whethre the i686 CPU can address > 2GB
of RAM.


Good point. I'll look into a patch implementing this logic.

Regards,
Jim


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [libvirt] Question about hypervisor that are not tristate

[Jim Fehlig]

On 01/11/2017 11:00 AM, Andrew Cooper wrote:

On 11/01/17 17:49, Jim Fehlig wrote:

On 01/06/2017 05:31 PM, Jim Fehlig wrote:

Adding xen-devel for a question below...


Happy new year!

Nearly a year ago I reported an issue with the  hypervisor
feature on Xen
[1] and am now seeing a similar issue with the  feature. Setting
the
default value of pae changed between xend and libxl. When not
specified, xend
would enable pae for HVM domains. Clients such as xm and the old
libvirt driver
did not have to explicitly enable it. In libxl, the pae field within
libxl_domain_build_info is initialized to 0. Clients must enable pae,
and indeed
xl will do so if pae=1 is not specified in the xl.cfg.

The xend behavior prevents libvirt from disabling pae, whereas the
libxl behvior
causes a guest ABI change (config that worked with libvirt+xend
doesn't with
libvirt+libxl). The libxl behavior also forces management software (e.g.
OpenStack nova) to add  where it wasn't needed before.

To solve this problem for , it was changed it to a tristate [2],
allowing
it to be turned off with explicit , and on if not
specified or
 or . Should  (and the remaining
hypervisor features
that are not tristate) be converted to tristate similar to ?
Alternatively,
I could simply set pae=1 for all HVM domains in the libxl driver.
Like the old
libvirt+xend behavior it couldn't be turned off, but I don't think
there is a
practical use-case to do so. At least no one has complained over all
the years
of libvirt+xend use.


Xen folks, what is your opinion of always enabling pae for HVM domains
in the libvirt libxl driver? Is there a need these days to disable it?

Jan had mentioned that some old, buggy guest OS's (Win 9x) might need
it disabled, and perhaps some cases where it may be desirable to
suppress a guest OS entering 64-bit mode. But in practice do you think
it is necessary to expose this knob to users?


ISTR the main use of this knob being to cause 32bit versions of windows
to avoid using PAE paging, making them more efficient to shadow.

Now that 64bit and EPT/NPT are fairly ubiquitous, there should be no
reason to turn it off.


Okay, thanks.



Can people still play with the CPUID policy if they really need to
disable it?


ATM, not through libvirt. We still have some work to do in this area.


It is unfortunate that this option was ever exposed via a
non-CPUID mechanism, but I am trying to clean this up at the hypervisor
interface to ensure that the *only* way to alter details like this is
via the appropriate interface.


And we'll need to make use of this interface in libvirt.

Regards,
Jim


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [libvirt] Question about hypervisor that are not tristate

[Jim Fehlig]

On 01/06/2017 05:31 PM, Jim Fehlig wrote:

Adding xen-devel for a question below...


Happy new year!

Nearly a year ago I reported an issue with the  hypervisor feature on Xen
[1] and am now seeing a similar issue with the  feature. Setting the
default value of pae changed between xend and libxl. When not specified, xend
would enable pae for HVM domains. Clients such as xm and the old libvirt driver
did not have to explicitly enable it. In libxl, the pae field within
libxl_domain_build_info is initialized to 0. Clients must enable pae, and indeed
xl will do so if pae=1 is not specified in the xl.cfg.

The xend behavior prevents libvirt from disabling pae, whereas the libxl behvior
causes a guest ABI change (config that worked with libvirt+xend doesn't with
libvirt+libxl). The libxl behavior also forces management software (e.g.
OpenStack nova) to add  where it wasn't needed before.

To solve this problem for , it was changed it to a tristate [2], allowing
it to be turned off with explicit , and on if not specified or
 or . Should  (and the remaining hypervisor features
that are not tristate) be converted to tristate similar to ? Alternatively,
I could simply set pae=1 for all HVM domains in the libxl driver. Like the old
libvirt+xend behavior it couldn't be turned off, but I don't think there is a
practical use-case to do so. At least no one has complained over all the years
of libvirt+xend use.


Xen folks, what is your opinion of always enabling pae for HVM domains in the 
libvirt libxl driver? Is there a need these days to disable it?


Jan had mentioned that some old, buggy guest OS's (Win 9x) might need it 
disabled, and perhaps some cases where it may be desirable to suppress a guest 
OS entering 64-bit mode. But in practice do you think it is necessary to expose 
this knob to users?


Thanks for your comments!

Regards,
Jim


[1] https://www.redhat.com/archives/libvir-list/2016-February/msg00197.html
[2] https://www.redhat.com/archives/libvir-list/2016-March/msg1.html



___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] tap device name for emulated NIC too long

[Jim Fehlig]

Hi All,

During the last Wg-openstack meetup we briefly discussed a long-standing bug 
when using Xen+libvirt+OpenStack with Neutron networking


https://bugs.launchpad.net/nova/+bug/1450465

The bug was also discussed on this list with no resolution

https://lists.xenproject.org/archives/html/xen-devel/2015-06/msg04116.html

To summarize: the tap device name for an emulated NIC is too long after libxl 
appends '-emu' to the name provided by Neutron. Some proposed fixes include


1. Shorten '-emu' to just '-e', avoiding IFNAMSIZ limit. But users are free to 
provide a name that already occupies the full IFNAMSIZ. Also, the user-provided 
name may be used in rules, filters, etc. elsewhere in the network, so modifying 
it at all seems questionable.


2. Change OpenStack to not exceed IFNAMSIZ-4 when specifying Xen vif name. This 
could be proposed to the Neutron devs, but IMO adding such Xen-specific hacks in 
OpenStack is undesirable.


3. Change the Xen default vif type from 'ioemu' to 'vif' (see 
docs/misc/xl-network-configuration.markdown), which avoids creating an emulated 
device. (Note: such a change could be made in Xen or libvirt.) But I think this 
is a no-go. I'd suspect it would result in a lot of broken configurations. E.g. 
a guest may not have PV drivers and is relying on the emulated device. Or the 
guest may be configured to network boot, in which case the emulated device would 
be needed for PXE [0].


We (the Wg-openstack folks) would like to hear your opinions on these proposals, 
or alternatives for fixing this bug.


Regards,
Jim

[0] iPXE claims support for Xen netfront devices, but I've not yet got it to 
work: http://lists.ipxe.org/pipermail/ipxe-devel/2014-July/003674.html


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] Opinions on removing the old, legacy libvirt Xen driver

[Jim Fehlig]

Hi All,

I briefly mentioned this at an evening event during the KVM Forum / Xen Dev 
Summit, but the list is certainly a better place to discuss such a topic. What 
do folks think about finally removing the old, legacy, xend-based driver from 
the libvirt sources?


The Xen community made xl/libxl the primary toolstack in Xen 4.1. In Xen 4.2, it 
was made the default toolstack. In Xen 4.5, xm/xend was completely removed from 
the Xen source tree. According to the Xen release support matrix [0], upstream 
maintenance of Xen 4.1-4.3 has been dropped for some time, including "long term" 
security support. Xen 4.4-4.5 no longer receive regular maintenance support, 
with security support ending in March for 4.4 and January 2018 for 4.5. In 
short, the fully maintained upstream Xen releases don't even contain xm/xend :-).


As for downstreams, I doubt anyone is interested in running the last several 
libvirt releases on an old Xen installition with xm/xend, let alone libvirt.git 
master. SUSE, which still supports Xen, has no interest in using a new libvirt 
on older (but still supported) SLES that uses the xm/xend toolstack. I struggle 
to find a good reason to keep any of the old cruft under src/xen/. I do think we 
should keep the xm/sexpr config parsing/formatting code src/xenconfig/ since it 
is useful for converting old xm and sexpr config to libvirt domXML.


Thanks for opinions and comments!

Regards,
Jim

[0] https://wiki.xenproject.org/wiki/Xen_Project_Release_Features

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 10/10] xl: allow to set the ratelimit value online for Credit2

[Jim Fehlig]

On 09/29/2016 08:54 PM, Dario Faggioli wrote:

Last part of the wiring necessary for allowing to
change the value of the ratelimit_us parameter online,
for Credit2 (like it is already for Credit1).

Signed-off-by: Dario Faggioli 
Reviewed-by: George Dunlap 


Seeing this patch got me to thinking that it would be cool if libvirt supported 
Credit2 :-). Do you have any plans/time to do that?


Regards,
Jim


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [OSSTEST PATCH 2/2] libvirt: Do not attempt save/restore when migration not advertised

[Jim Fehlig]

On 10/04/2016 11:02 AM, Ian Jackson wrote:

Currently, osstest wrongly thinks that ARM can do save/restore,
because `virsh help' does mention the save command (on all
architectures).

Additionally, check the virth capabilities xpath
   /capabilities/host/migration_features
to try to see whether this host supports migration.

I am not sure if this is the right path to check.  Perhaps
   /capabilities/host/migration_features/live
is more correct, but this may be wrong if Xen comes to support save/restore
on ARM, but not live migration (but perhaps libvirt cannot express this
distinction in which case perhaps it's right after all).


Looking at the capabilities generation code again, I see that 
virCapabilitiesNew() takes 'offlineMigrate' and 'liveMigrate' parameters. I 
assume offline in this context means save, copy, restore. Martin, is that 
assumption correct?


If so, I think the saverestore_check() below can look for
/capabilities/host/migration_features. The migration check in 1/2 can look for
/capabilities/host/migration_features/live. Is it fair to assume save/restore is 
available when live migration is supported?


Regards,
Jim



Signed-off-by: Ian Jackson <ian.jack...@eu.citrix.com>
CC: Julien Grall <julien.gr...@arm.com>
CC: Jim Fehlig <jfeh...@suse.com>
---
 Osstest/Toolstack/libvirt.pm | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Osstest/Toolstack/libvirt.pm b/Osstest/Toolstack/libvirt.pm
index b7db7af..250fe47 100644
--- a/Osstest/Toolstack/libvirt.pm
+++ b/Osstest/Toolstack/libvirt.pm
@@ -111,7 +111,9 @@ sub check_for_command($$) {

 sub saverestore_check ($) {
 my ($self) = @_;
-return check_for_command($self, "save");
+return
+   _check_capability($self, '/capabilities/host/migration_features') &&
+   check_for_command($self, "save");
 }

 sub migrate () {




___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [OSSTEST PATCH 1/2] libvirt: Check migration capabilities using proper XML parser

[Jim Fehlig]

On 10/04/2016 11:02 AM, Ian Jackson wrote:

Do not grep the virsh capabilities output (!)  Instead, parse the XML
using perl's XML modules and look for the specific feature flag using
an XPATH pattern.

AFAICT from looking at the XML, that's


  

  

But the original code does not test for .

Xen could in principle (and AIUI might in the future, on ARM) support
save/restore but not live migration.  Currently it supports neither.

I don't know whether libvirt's capabilities system can capture this
distinction.  libvirt.git#1d37a4c4 "libxl: detect support for save and
restore" suggests not.


That's correct. But as Martin suggested in 2/2, libvirt could be enhanced to 
report the distinction if/when needed.




Perhaps relatedly, I am not sure whether this test should be changed
to look for the xpath
  /capabilities/host/migration_features/live
instead.  The schema (libvirt.git/docs/schemas/capability.rng) seems
to suggest that it probably should.


Agreed. migrate_check() should look for
/capabilities/host/migration_features/live.

Regards,
Jim



For now, this osstest commit has no ultimate functional change (with
libvirt output as it currently appears to be on real hosts).

Signed-off-by: Ian Jackson <ian.jack...@eu.citrix.com>
CC: Julien Grall <julien.gr...@arm.com>
CC: Jim Fehlig <jfeh...@suse.com>
---
 Osstest/Toolstack/libvirt.pm | 17 ++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/Osstest/Toolstack/libvirt.pm b/Osstest/Toolstack/libvirt.pm
index 69ff0bb..b7db7af 100644
--- a/Osstest/Toolstack/libvirt.pm
+++ b/Osstest/Toolstack/libvirt.pm
@@ -21,6 +21,8 @@ use strict;
 use warnings;

 use Osstest::TestSupport;
+use XML::LibXML::XPathContext;
+use XML::LibXML;

 sub new {
 my ($class, $ho, $methname,$asset) = @_;
@@ -72,6 +74,17 @@ sub shutdown_wait ($$$) {
 guest_await_destroy($gho,$timeout);
 }

+sub _check_capability ($$) {
+my ($self, $xpath) = @_;
+my $ho = $self->{Host};
+my $caps = target_cmd_output_root($ho, 'virsh capabilities');
+my $stash = open_unique_stashfile('virsh-capabilities');
+my $dom = XML::LibXML->load_xml(string => $caps);
+my $xc = XML::LibXML::XPathContext->new($dom);
+my @nodes = $xc->findnodes($xpath);
+return !!@nodes;
+}
+
 sub migrate_check ($$) {
 my ($self, $local) = @_;
 my $rc;
@@ -80,9 +93,7 @@ sub migrate_check ($$) {
 # local migration is not supported
 $rc = 1;
 } else {
-   my $ho = $self->{Host};
-   my $caps = target_cmd_output_root($ho, "virsh capabilities");
-   $rc = ($caps =~ m//) ? 0 : 1
+   $rc = $self->check_capability('/capabilities/host/migration_features');
 }

 logm("rc=$rc");




___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [PATCH] docs: define semantics of vncpasswd in xl.cfg

[Jim Fehlig]

A recent discussion around LSN-2016-0001 [1] included defining
the sematics of an empty string for a VNC password. It was stated
that "libxl interprets an empty password in the caller's
configuration to mean that passwordless access should be permitted".

The same applies for vncpasswd setting in xl.cfg. This patch
extends to xl.cfg documentation to define the semantics of setting
vncpasswd to an empty string.

Signed-off-by: Jim Fehlig <jfeh...@suse.com>
---
 docs/man/xl.cfg.pod.5.in | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/docs/man/xl.cfg.pod.5.in b/docs/man/xl.cfg.pod.5.in
index 3bb27d0..48c9c0d 100644
--- a/docs/man/xl.cfg.pod.5.in
+++ b/docs/man/xl.cfg.pod.5.in
@@ -561,7 +561,9 @@ The actual display used can be accessed with C.
 
 =item C

Re: [Xen-devel] [libvirt] Fixing libvirt's libxl driver breakage -- where to define LIBXL_API_VERSION?

[Jim Fehlig]

On 06/27/2016 10:12 AM, Ian Jackson wrote:

Daniel P. Berrange writes ("Re: [libvirt] [Xen-devel] Fixing libvirt's libxl driver 
breakage -- where to define LIBXL_API_VERSION?"):

On Mon, Jun 27, 2016 at 04:54:35PM +0100, Ian Jackson wrote:

Created the following branch refs on xenbits in the toplevel
libvirt.git:

  osstest/frozen/xen-4.3-testing 9a0c7f5f834185db9017c34aabc03ad99cf37bed
  osstest/frozen/xen-4.4-testing 33fb8ff185846a7b4974105d2c9400690a6f95cf
  osstest/frozen/xen-4.5-testing cda1cc170f07b45911b3dad03e42c8ebfc210fa1
  osstest/frozen/xen-4.6-testing eac167e2610d3e59b32f7ec7ba78cbc8c420a425
  osstest/frozen/xen-4.7-testing 1a41ed5af5e1704dd9b0bdca40df5c9cacbdabfc

How did you pick those hashes ? Would it make more sense to pick the
nearest libvirt release tag ? eg v1.3.2 instead of 33fb8ff18584 ?


These were those tested by the following `tolerable' osstest push gate
flights for the corresponding Xen tree:

  xen-4.3-testing 9a0c7f5f8341 86673
  xen-4.4-testing 33fb8ff18584 85031
  xen-4.5-testing cda1cc170f07 83135
  xen-4.6-testing eac167e2610d 96031
  xen-4.7-testing 1a41ed5af5e1 95728

I picked them by searching my mail archives for osstest `tolerable'
push gate flights - ie, passes in our CI system.

That minimises the risk that the selected versions are themselves
troublesome for some reason, needing another round of adjustment.

It might indeed be better to convert them to nearby release tags.
However:

mariner:libvirt> git-describe 9a0c7f5f834185db9017c34aabc03ad99cf37bed
v1.3.2-202-g9a0c7f5
mariner:libvirt> git-describe 33fb8ff185846a7b4974105d2c9400690a6f95cf
v1.3.2-rc2-1-g33fb8ff
mariner:libvirt> git-describe cda1cc170f07b45911b3dad03e42c8ebfc210fa1
v1.3.1-262-gcda1cc1


It seems odd that Xen 4.5 would use an older libvirt release than Xen 4.3.


mariner:libvirt> git-describe eac167e2610d3e59b32f7ec7ba78cbc8c420a425
v1.3.5-318-geac167e
mariner:libvirt> git-describe 1a41ed5af5e1704dd9b0bdca40df5c9cacbdabfc
v1.3.5-129-g1a41ed5
mariner:libvirt>

So in most cases these hashes are well away from a release tag.

Does libvirt have stable release branches ?  One approach would be to
have osstest track a suitable libvirt stable release branche for each
Xen stable release branch.


I see Daniel already answered this question.



That would involve setting up a push gate for each of the chosen
libvirt stable branches.  That would be worthwhile if we expect those
stable branches to acquire commits which break Xen, and which we could
like to be told about.  But I'm not sure that's the case.


I occasionally backport Xen bug fixes to -maint branches. Cole has also grabbed 
some Xen bug fixes when making a stable release of a -maint branch. But such 
backports should be trivial and obvious bug fixes that shouldn't cause build or 
runtime breakage with Xen.


Regards,
Jim


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] XSA-180 follow-up: repurpose xenconsoled for logging

[Jim Fehlig]

On 06/21/2016 09:53 AM, George Dunlap wrote:
> On 21/06/16 16:11, Ian Jackson wrote:
>> Wei Liu writes ("Re: XSA-180 follow-up: repurpose xenconsoled for logging"):
>>> Here is what we have gathered so far:
>> ...
>>> We can, however, just make recommendation that system administrators can
>>> easily set up and call it a day. There are suggestions that we can
>>> recommend conserver or sympathy, but I haven't seen a concrete viable
>>> proposal yet. What I hope is that we can have a document in tree in the
>>> end.
>> sympathy would need some extra engineering to become suitable.  It's
>> also not widely adopted.  (Not even in Debian, yet.  Sorry about that,
>> but in my defence it's not my project...)
>>
>>> Another way is to invent our own "virtlogd" -- it could be a new daemon,
>>> it could be xenconsoled. The major concern is that we're adding a
>>> critical component to the system and it may not scale well. We can make
>>> a compromise by using non-blocking fd to make the new component less
>>> critical and doesn't hinder scalability.
>> I think this is probably the best answer.  We already have most of
>> this in the form of xenconsoled.
>>
>>> Another way is to alter libxl API and ask the application to pass in a
>>> fd for logging. The major concern is that this is not suitable in the
>>> context of a security issue.
>> Any solution needs to work for xl as well as other users of libxl.  So
>> this is not a description of a solution option; rather it is a
>> proposal to move the functionality/glue/problem/whatever out of libxl
>> into xl.
> ...or libvirt, or xapi (should it ever be ported to libxl).
>
> I think that having the option to pass an fd in would be useful and will
> probably be wanted at some point; I think libvirt for instance should
> probably be modified to use such an interface once it's available to
> connect qemu to virtlogd.

It would be easy enough to do since the qemu driver is already doing this.

Regards,
Jim


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [libvirt test] 95577: regressions - FAIL

[Jim Fehlig]

On 06/12/2016 11:01 PM, osstest service owner wrote:
> flight 95577 libvirt real [real]
> http://logs.test-lab.xenproject.org/osstest/logs/95577/
>
> Regressions :-(
>
> Tests which did not succeed and are blocking,
> including tests which could not be run:
>  build-i386-libvirt5 libvirt-build fail REGR. vs. 
> 95460
>  build-amd64-libvirt   5 libvirt-build fail REGR. vs. 
> 95460
>  build-armhf-libvirt   5 libvirt-build fail REGR. vs. 
> 95460

I see you've already fixed this Wei. Thanks!

Regards,
Jim


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [libvirt test] 94692: regressions - FAIL

[Jim Fehlig]

On 05/22/2016 02:12 AM, osstest service owner wrote:
> flight 94692 libvirt real [real]
> http://logs.test-lab.xenproject.org/osstest/logs/94692/
>
> Regressions :-(
>
> Tests which did not succeed and are blocking,
> including tests which could not be run:
>  build-i386-libvirt5 libvirt-build fail REGR. vs. 
> 94591
>  build-amd64-libvirt   5 libvirt-build fail REGR. vs. 
> 94591
>  build-armhf-libvirt   5 libvirt-build fail REGR. vs. 
> 94591

These failures should already be fixed by

http://libvirt.org/git/?p=libvirt.git;a=commit;h=33705aeec0f8b87ccb4c06798679583ec9c3327e

Regards,
Jim


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH V2] libxl: don't add cache mode for qdisk cdrom drives

[Jim Fehlig]

On 05/10/2016 04:06 AM, Stefano Stabellini wrote:
> On Mon, 9 May 2016, Wei Liu wrote:
>> On Thu, Apr 28, 2016 at 03:20:46PM -0600, Jim Fehlig wrote:
>>> qemu commit 91a097e7 forbids specifying cache mode for empty
>>> drives. Attempting to create a domain with an empty qdisk cdrom
>>> drive results in
>>>
>>> qemu-system-x86_64: -drive if=ide,index=1,readonly=on,media=cdrom,
>>>cache=writeback,id=ide-832: Must specify either driver or file
>>>
>> We need to fix this one way or another.
>>
>>> libxl only allows an empty 'target=' for cdroms. By default, cdroms
>>> are readonly (see the 'access' parameter in xl-disk-configuration.txt)
>>> and forced to readonly by any tools (e.g. xl) using libxlutil's
>>> xlu_disk_parse() function. With cdroms always marked readonly,
>>> explicitly specifying the cache mode for cdrom drives can be dropped.
>>> The drive's 'readonly=on' option can also be set unconditionally.
>>>
>>> Signed-off-by: Jim Fehlig <jfeh...@suse.com>
>> CC Stefano who made the change to use writeback in
>> e9a327bbbcab127625b0917a2780cb3601a81d01 . Also CC Anthony.
>>
>> Ian, Stefano and Anthony, do you have opinion on this patch?
> It looks good to me. We could consider to backport it, given that we
> only have a loose relationship with QEMU and older versions of Xen could
> end up running with newer versions of QEMU.

Wei,

Will this patch make 4.7? It allows empty cdroms with freshly released QEMU 2.6 
:-).

Regards,
Jim


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [libvirt] [PATCH 1/3] libxl: switch to using libxl_domain_create_restore from v4.4 API

[Jim Fehlig]

Olaf Hering wrote:
> On Wed, May 11, Jim Fehlig wrote:
> 
>> https://www.redhat.com/archives/libvir-list/2016-May/msg00686.html
> 
> Perhaps configure.ac should be changed to reject libxl upfront.

Yes, good idea.

> It looks
> like the assignment of LIBXL_CFLAGS should be moved up, so that
> AC_CHECK_LIB gets the -DLIBXL_API_VERSION as well. I will provide a
> patch for that.

Ok, thanks for volunteering!

Regards,
Jim

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 1/3] libxl: switch to using libxl_domain_create_restore from v4.4 API

[Jim Fehlig]

On 05/11/2016 12:01 AM, Olaf Hering wrote:
> On Mon, May 02, Jim Fehlig wrote:
>
>> In LIBXL_API_VERSION 0x040400, the libxl_domain_create_restore API
>> gained a parameter for specifying restore parameters. Switch to
>> using version 0x040400, which will be useful in a subsequent commit
>> to specify the Xen migration stream version when restoring.
> This breaks compilation with xen-4.3 etc. Is this intentional?

Yes. Did you see my message elsewhere in the thread when I notified I was
pushing the series?

https://www.redhat.com/archives/libvir-list/2016-May/msg00686.html

Regards,
Jim


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [libvirt] [PATCH 0/3] libxl: support Xen migration stream V2

[Jim Fehlig]

Ján Tomko wrote:
> On Mon, May 02, 2016 at 07:01:16PM -0600, Jim Fehlig wrote:
>> Hi all,
>>
>> This patch adds support for Xen migration stream V2 to the libvirt
>> libxl driver. In the process it fixes save/restore and migration
>> tests in OSSTest, which have been failing since libvirt commit
>> e7440656.
>>
>> Patch1 changes the libxl API requirement from version 4.2 to 4.4,
>> enabling use of an extended libxl_domain_create_restore() function
>> that allows passing restore parameters such as stream version.
>>
>> Patch2 adds support for migration stream V2 in the basic save/restore
>> logic, taking advantage of a save image header that includes a version
>> field. The version is set to '2' if the host produces a V2 migration
>> stream. This patch fixes the failing save/restore tests in OSSTest.
>>
>> Patch3 adds support for migration stream V2 in the migration logic.
>> The migration code does not use the save image header and instead
>> uses libvirt's migration protocol to transfer domain configuration
>> and other such goodies from source to destination. The patch
>> enables sending version information in the Begin and Prepare
>> migration phases so the correct stream version information can be
>> passed to libxl_domain_create_restore(). An upshot of this approach
>> is safely detecting and aborting a migration from a V2 host to a
>> V1-only host. This patch fixes the failing migration tests in
>> OSSTest.
>>
>> Jim Fehlig (3):
>>   libxl: switch to using libxl_domain_create_restore from v4.4 API
>>   libxl: support Xen migration stream V2 in save/restore
>>   libxl: support migration stream V2 in migration
>>
>>  configure.ac|   9 +-
>>  src/libxl/libxl_conf.h  |   6 +-
>>  src/libxl/libxl_domain.c|  46 --
>>  src/libxl/libxl_domain.h|  14 ++-
>>  src/libxl/libxl_driver.c|  27 +++---
>>  src/libxl/libxl_migration.c | 204 
>> +++-
>>  src/libxl/libxl_migration.h |   6 +-
>>  7 files changed, 282 insertions(+), 30 deletions(-)
> 
> ACK series

Thanks, pushed now.

Note to all: With this change, libvirt.git will no longer build with Xen < 4.4.
Wearing either upstream maintainer or distro packager hat, I don't really care 
:-).

With upstream hat on, Xen 4.2 is no longer maintained. Xen 4.3 saw the end of
general support in Jan 2015 and will end the subsequent 18 months of security
support in early July. So in a little over a month 4.3 will be dead.

With distro packager hat on, I'm not interested in using a new libvirt with such
an old Xen. I hope others feel the same. If not, patches are welcome.

Regards,
Jim


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 0/3] libxl: support Xen migration stream V2

[Jim Fehlig]

On 05/03/2016 07:19 AM, Wei Liu wrote:
> On Mon, May 02, 2016 at 07:01:16PM -0600, Jim Fehlig wrote:
>> Hi all,
>>
>> This patch adds support for Xen migration stream V2 to the libvirt
>> libxl driver. In the process it fixes save/restore and migration
>> tests in OSSTest, which have been failing since libvirt commit
>> e7440656.
>>
>> Patch1 changes the libxl API requirement from version 4.2 to 4.4,
>> enabling use of an extended libxl_domain_create_restore() function
>> that allows passing restore parameters such as stream version.
>>
>> Patch2 adds support for migration stream V2 in the basic save/restore
>> logic, taking advantage of a save image header that includes a version
>> field. The version is set to '2' if the host produces a V2 migration
>> stream. This patch fixes the failing save/restore tests in OSSTest.
>>
>> Patch3 adds support for migration stream V2 in the migration logic.
>> The migration code does not use the save image header and instead
>> uses libvirt's migration protocol to transfer domain configuration
>> and other such goodies from source to destination. The patch
>> enables sending version information in the Begin and Prepare
>> migration phases so the correct stream version information can be
>> passed to libxl_domain_create_restore(). An upshot of this approach
>> is safely detecting and aborting a migration from a V2 host to a
>> V1-only host. This patch fixes the failing migration tests in
>> OSSTest.
>>
> The general approach looks good to me.

Any comments on this series from libvirt maintainers? Would be nice to get this
series (or a variant) committed, fixing the daily OSSTest failures. For your
viewing pleasure, here's a link to today's failure

http://logs.test-lab.xenproject.org/osstest/logs/93880/

Regards,
Jim


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] tools: Restrict configuration of qemu processes

[Jim Fehlig]

On 05/09/2016 10:35 AM, Ian Jackson wrote:
> Ian Jackson writes ("Re: [Xen-devel] [PATCH] tools: Restrict configuration of 
> qemu processes"):
>> Jim Fehlig writes ("[Xen-devel] [PATCH] tools: Restrict configuration of 
>> qemu processes"):
>>> Commit 6ef823fd added '-nodefaults' to the qemu args created by
>>> libxl, which is a good step in restricting qemu's default
>>> configuration. This change takes another step by adding
>>> -no-user-config, which ignores any user-provided config files in
>>> sysconfdir. Together, -nodefaults and -no-user-config allow Xen
>>> to avoid unkown and uncontrolled qemu configuration.
>>>
>>> Both options are also added to the qemu invocation in the
>>> xen-qemu-dom0-disk-backend systemd service file.
>> Queued, thanks.  Also listed for backport.
> I found this on my backport todo list.  Thinking about it, I have had
> second thoughts.
>
> I worry that existing users of stable branches might be relying on the
> user config feature (for example by dropping qemu configuration in
> ~root).  If they are, then applying this would break things for them.
>
> It's not a security problem because in xen the configuration in
> question would have to come from ~root.

Good point.

> So I think, probably, that we should leave this be (ie, not backport
> the patch).  Does anyone want to try to change my mind ?

I never asked for a backport, so have no incentive to change your mind. Plus, I
agree with your comment.

Regards,
Jim


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 1/3] libxl: switch to using libxl_domain_create_restore from v4.4 API

[Jim Fehlig]

In LIBXL_API_VERSION 0x040400, the libxl_domain_create_restore API
gained a parameter for specifying restore parameters. Switch to
using version 0x040400, which will be useful in a subsequent commit
to specify the Xen migration stream version when restoring.

Signed-off-by: Jim Fehlig <jfeh...@suse.com>
---
 configure.ac | 9 +
 src/libxl/libxl_domain.c | 6 +-
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/configure.ac b/configure.ac
index 88e2e20..0da0f75 100644
--- a/configure.ac
+++ b/configure.ac
@@ -875,10 +875,11 @@ if test "$with_libxl" != "no" ; then
 fi
 fi
 
-# Until there is a need to use enhancements of libxl APIs such as
-# libxl_domain_create_restore and libxl_set_vcpuaffinity, stick with
-# the APIs as defined in libxl API version 4.2.0.
-LIBXL_CFLAGS="$LIBXL_CFLAGS -DLIBXL_API_VERSION=0x040200"
+# LIBXL_API_VERSION 4.4.0 introduced a new parameter to
+# libxl_domain_create_restore for specifying restore parameters.
+# The libxl driver will make use of this new parameter for specifying
+# the Xen migration stream version.
+LIBXL_CFLAGS="$LIBXL_CFLAGS -DLIBXL_API_VERSION=0x040400"
 LIBS="$old_LIBS"
 CFLAGS="$old_CFLAGS"
 
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index 14a900c..32ad946 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -1028,6 +1028,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver, 
virDomainObjPtr vm,
 libxlDriverConfigPtr cfg;
 virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr;
 libxl_asyncprogress_how aop_console_how;
+libxl_domain_restore_params params;
 
 libxl_domain_config_init(_config);
 
@@ -1115,8 +1116,11 @@ libxlDomainStart(libxlDriverPrivatePtr driver, 
virDomainObjPtr vm,
 ret = libxl_domain_create_new(cfg->ctx, _config,
   , NULL, _console_how);
 } else {
+libxl_domain_restore_params_init();
 ret = libxl_domain_create_restore(cfg->ctx, _config, ,
-  restore_fd, NULL, _console_how);
+  restore_fd, , NULL,
+  _console_how);
+libxl_domain_restore_params_dispose();
 }
 virObjectLock(vm);
 
-- 
2.1.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 3/3] libxl: support migration stream V2 in migration

[Jim Fehlig]

Similar to "support Xen migration stream V2 in save/restore",
add support for indicating the migration stream version in
the migration code. To accomplish this, add a minimal migration
cookie in the libxl driver that is passed between source and
destination hosts. Initially, the cookie is only used in
the Begin and Prepare phases of migration to communicate the
version of the migration stream produced by the source.

Signed-off-by: Jim Fehlig <jfeh...@suse.com>
---
 src/libxl/libxl_driver.c|  14 +--
 src/libxl/libxl_migration.c | 204 +++-
 src/libxl/libxl_migration.h |   6 +-
 3 files changed, 213 insertions(+), 11 deletions(-)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 8977ae2..062d6f8 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -5151,8 +5151,8 @@ static char *
 libxlDomainMigrateBegin3Params(virDomainPtr domain,
virTypedParameterPtr params,
int nparams,
-   char **cookieout ATTRIBUTE_UNUSED,
-   int *cookieoutlen ATTRIBUTE_UNUSED,
+   char **cookieout,
+   int *cookieoutlen,
unsigned int flags)
 {
 const char *xmlin = NULL;
@@ -5193,15 +5193,16 @@ libxlDomainMigrateBegin3Params(virDomainPtr domain,
 return NULL;
 }
 
-return libxlDomainMigrationBegin(domain->conn, vm, xmlin);
+return libxlDomainMigrationBegin(domain->conn, vm, xmlin,
+ cookieout, cookieoutlen);
 }
 
 static int
 libxlDomainMigratePrepare3Params(virConnectPtr dconn,
  virTypedParameterPtr params,
  int nparams,
- const char *cookiein ATTRIBUTE_UNUSED,
- int cookieinlen ATTRIBUTE_UNUSED,
+ const char *cookiein,
+ int cookieinlen,
  char **cookieout ATTRIBUTE_UNUSED,
  int *cookieoutlen ATTRIBUTE_UNUSED,
  char **uri_out,
@@ -5240,7 +5241,8 @@ libxlDomainMigratePrepare3Params(virConnectPtr dconn,
 if (virDomainMigratePrepare3ParamsEnsureACL(dconn, def) < 0)
 goto error;
 
-if (libxlDomainMigrationPrepare(dconn, , uri_in, uri_out, flags) < 0)
+if (libxlDomainMigrationPrepare(dconn, , uri_in, uri_out,
+cookiein, cookieinlen, flags) < 0)
 goto error;
 
 return 0;
diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
index 1d4ec5e..ad54960 100644
--- a/src/libxl/libxl_migration.c
+++ b/src/libxl/libxl_migration.c
@@ -48,6 +48,18 @@
 
 VIR_LOG_INIT("libxl.libxl_migration");
 
+typedef struct _libxlMigrationCookie libxlMigrationCookie;
+typedef libxlMigrationCookie *libxlMigrationCookiePtr;
+struct _libxlMigrationCookie {
+/* Host properties */
+char *srcHostname;
+uint32_t xenMigStreamVer;
+
+/* Guest properties */
+unsigned char uuid[VIR_UUID_BUFLEN];
+char *name;
+};
+
 typedef struct _libxlMigrationDstArgs {
 virObject parent;
 
@@ -55,6 +67,7 @@ typedef struct _libxlMigrationDstArgs {
 virConnectPtr conn;
 virDomainObjPtr vm;
 unsigned int flags;
+libxlMigrationCookiePtr migcookie;
 
 /* for freeing listen sockets */
 virNetSocketPtr *socks;
@@ -63,11 +76,166 @@ typedef struct _libxlMigrationDstArgs {
 
 static virClassPtr libxlMigrationDstArgsClass;
 
+
+static void
+libxlMigrationCookieFree(libxlMigrationCookiePtr mig)
+{
+if (!mig)
+return;
+
+VIR_FREE(mig->srcHostname);
+VIR_FREE(mig->name);
+VIR_FREE(mig);
+}
+
+static libxlMigrationCookiePtr
+libxlMigrationCookieNew(virDomainObjPtr dom)
+{
+libxlMigrationCookiePtr mig = NULL;
+
+if (VIR_ALLOC(mig) < 0)
+goto error;
+
+if (VIR_STRDUP(mig->name, dom->def->name) < 0)
+goto error;
+
+memcpy(mig->uuid, dom->def->uuid, VIR_UUID_BUFLEN);
+
+if (!(mig->srcHostname = virGetHostname()))
+goto error;
+
+mig->xenMigStreamVer = LIBXL_SAVE_VERSION;
+
+return mig;
+
+ error:
+libxlMigrationCookieFree(mig);
+return NULL;
+}
+
+
+static int
+libxlMigrationBakeCookie(libxlMigrationCookiePtr mig,
+ char **cookieout,
+ int *cookieoutlen)
+{
+virBuffer buf = VIR_BUFFER_INITIALIZER;
+char uuidstr[VIR_UUID_STRING_BUFLEN];
+
+if (!cookieout || !cookieoutlen)
+return 0;
+
+*cookieoutlen = 0;
+virUUIDFormat(mig->uuid, uuidstr);
+
+virBufferAddLit(, "\n");
+virBufferAdjustIndent(, 2);
+virBufferEscapeString(, "%s\n", mig->name);
+virBufferAsprintf(, "%s\

[Xen-devel] [PATCH 2/3] libxl: support Xen migration stream V2 in save/restore

[Jim Fehlig]

Xen 4.6 introduced a new migration stream commonly referred to as
"migration V2". Xen 4.6 and newer always produce this new stream,
whereas Xen 4.5 and older always produce the legacy stream.
Support for migration stream V2 can be detected at build time with
LIBXL_HAVE_SRM_V2 from libxl.h. The legacy and V2 streams are not
compatible, but a V2 host can accept and convert a legacy stream.

Commit e7440656 changed the libxl driver to use the lowest libxl
API version possible (version 0x040200) to ensure the driver
builds against older Xen releases. The old 4.2 restore API does
not support specifying a stream version and assumes a legacy
stream, even if the incoming stream is migration V2. Thinking it
has been given a legacy stream, libxl will fail to convert an
incoming stream that is already V2, which causes the entire
restore operation to fail. Xen's libvirt-related OSSTest has been
failing since commit e7440656 landed in libvirt.git master. One
of the more recent failures can be seen here

http://lists.xenproject.org/archives/html/xen-devel/2016-05/msg00071.html

This patch changes the call to libxl_domain_create_restore() to
include the stream version if LIBXL_HAVE_SRM_V2 is defined. The
version field of the libxlSavefileHeader struct is also updated
to '2' when LIBXL_HAVE_SRM_V2 is defined, ensuring the stream
version in the header matches the actual stream version produced
by Xen. Along with bumping the libxl API requirement to 0x040400,
this patch fixes save/restore on a migration V2 Xen host.

Oddly, migration has never used the libxlSavefileHeader. It
handles passing configuration in the Begin and Prepare phases,
and then calls libxl directly to transfer domain state/memory
in the Perform phase. A subsequent patch will add stream
version handling in the Begin and Prepare phase handshaking,
which will fix the migration related OSSTest failures.

Signed-off-by: Jim Fehlig <jfeh...@suse.com>
---
 src/libxl/libxl_conf.h  |  6 +-
 src/libxl/libxl_domain.c| 40 
 src/libxl/libxl_domain.h| 14 ++
 src/libxl/libxl_driver.c| 13 -
 src/libxl/libxl_migration.c |  2 +-
 5 files changed, 60 insertions(+), 15 deletions(-)

diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h
index 24e2911..c5b9429 100644
--- a/src/libxl/libxl_conf.h
+++ b/src/libxl/libxl_conf.h
@@ -148,7 +148,11 @@ struct _libxlDriverPrivate {
 };
 
 # define LIBXL_SAVE_MAGIC "libvirt-xml\n \0 \r"
-# define LIBXL_SAVE_VERSION 1
+# ifdef LIBXL_HAVE_SRM_V2
+#  define LIBXL_SAVE_VERSION 2
+# else
+#  define LIBXL_SAVE_VERSION 1
+# endif
 
 typedef struct _libxlSavefileHeader libxlSavefileHeader;
 typedef libxlSavefileHeader *libxlSavefileHeaderPtr;
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index 32ad946..5fa1bd9 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -514,7 +514,7 @@ libxlDomainShutdownThread(void *opaque)
 }
 libxlDomainDestroyInternal(driver, vm);
 libxlDomainCleanup(driver, vm);
-if (libxlDomainStart(driver, vm, false, -1) < 0) {
+if (libxlDomainStartNew(driver, vm, false) < 0) {
 virErrorPtr err = virGetLastError();
 VIR_ERROR(_("Failed to restart VM '%s': %s"),
   vm->def->name, err ? err->message : _("unknown error"));
@@ -1006,14 +1006,23 @@ libxlDomainCreateIfaceNames(virDomainDefPtr def, 
libxl_domain_config *d_config)
 }
 
 
+#ifdef LIBXL_HAVE_SRM_V2
+# define LIBXL_DOMSTART_RESTORE_VER_ATTR /* empty */
+#else
+# define LIBXL_DOMSTART_RESTORE_VER_ATTR ATTRIBUTE_UNUSED
+#endif
+
 /*
  * Start a domain through libxenlight.
  *
  * virDomainObjPtr must be locked and a job acquired on invocation
  */
-int
-libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
- bool start_paused, int restore_fd)
+static int
+libxlDomainStart(libxlDriverPrivatePtr driver,
+ virDomainObjPtr vm,
+ bool start_paused,
+ int restore_fd,
+ uint32_t restore_ver LIBXL_DOMSTART_RESTORE_VER_ATTR)
 {
 libxl_domain_config d_config;
 virDomainDefPtr def = NULL;
@@ -1049,6 +1058,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver, 
virDomainObjPtr vm,
 goto cleanup;
 
 restore_fd = managed_save_fd;
+restore_ver = hdr.version;
 
 if (STRNEQ(vm->def->name, def->name) ||
 memcmp(vm->def->uuid, def->uuid, VIR_UUID_BUFLEN)) {
@@ -1117,6 +1127,9 @@ libxlDomainStart(libxlDriverPrivatePtr driver, 
virDomainObjPtr vm,
   , NULL, _console_how);
 } else {
 libxl_domain_restore_params_init();
+#ifdef LIBXL_HAVE_SRM_V2
+params.stream_version = restore_ver;
+#endif
 ret = libxl_domain_create_restore(cfg->ctx, _config, ,
   restore_fd, , N

[Xen-devel] [PATCH 0/3] libxl: support Xen migration stream V2

[Jim Fehlig]

Hi all,

This patch adds support for Xen migration stream V2 to the libvirt
libxl driver. In the process it fixes save/restore and migration
tests in OSSTest, which have been failing since libvirt commit
e7440656.

Patch1 changes the libxl API requirement from version 4.2 to 4.4,
enabling use of an extended libxl_domain_create_restore() function
that allows passing restore parameters such as stream version.

Patch2 adds support for migration stream V2 in the basic save/restore
logic, taking advantage of a save image header that includes a version
field. The version is set to '2' if the host produces a V2 migration
stream. This patch fixes the failing save/restore tests in OSSTest.

Patch3 adds support for migration stream V2 in the migration logic.
The migration code does not use the save image header and instead
uses libvirt's migration protocol to transfer domain configuration
and other such goodies from source to destination. The patch
enables sending version information in the Begin and Prepare
migration phases so the correct stream version information can be
passed to libxl_domain_create_restore(). An upshot of this approach
is safely detecting and aborting a migration from a V2 host to a
V1-only host. This patch fixes the failing migration tests in
OSSTest.

Jim Fehlig (3):
  libxl: switch to using libxl_domain_create_restore from v4.4 API
  libxl: support Xen migration stream V2 in save/restore
  libxl: support migration stream V2 in migration

 configure.ac|   9 +-
 src/libxl/libxl_conf.h  |   6 +-
 src/libxl/libxl_domain.c|  46 --
 src/libxl/libxl_domain.h|  14 ++-
 src/libxl/libxl_driver.c|  27 +++---
 src/libxl/libxl_migration.c | 204 +++-
 src/libxl/libxl_migration.h |   6 +-
 7 files changed, 282 insertions(+), 30 deletions(-)

-- 
2.1.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] libxl: don't add cache mode for empty drives

[Jim Fehlig]

Roger Pau Monné wrote:
> On Thu, Apr 28, 2016 at 09:27:30AM +0100, George Dunlap wrote:
>> On Wed, Apr 27, 2016 at 5:22 PM, Jim Fehlig <jfeh...@suse.com> wrote:
>>> On 04/27/2016 01:38 AM, Roger Pau Monné wrote:
>>>> On Tue, Apr 26, 2016 at 10:35:31PM -0600, Jim Fehlig wrote:
>>>>> qemu commit 91a097e7 forbids specifying the cache mode for empty
>>>>> drives. Attempting to create a domain with an empty qdisk cdrom
>>>>> results in
>>>>>
>>>>> qemu-system-x86_64: -drive if=ide,index=1,readonly=on,media=cdrom,
>>>>>cache=writeback,id=ide-832: Must specify either driver or file
>>>>>
>>>>> Change libxl to only emit cache mode when a cdrom target is specified.
>>>> What happens then when a cdrom is inserted? I cannot seem to find the code
>>>> in libxl_cdrom_insert that sets the cache mode.
>>> I cannot find it either. I suppose it would need to be setup via xenstore,
>>> similar to other options like feature_discard. But looking at
>>> $qemu-src/hw/block/xen_disk.c, it seems the XenBlkDev struct has no field to
>>> specify cache mode. Would qemu's xen_disk need to be extended to support 
>>> cache
>>> mode, followed by a libxl patch to set the cache mode in xenstore?
>>>
>>>>  Is the default one used
>>>> then?
>>> Yes, the default cache mode (which is already writeback AIUI) would be used 
>>> if
>>> not explicitly specified. Which brings up the option of removing
>>> 'cache=writeback' for cdroms altogether. Any opinion on that option?
>> What's the effective difference between caching modes for read-only
>> media anyway?
> 
> That's right, cdroms should always be read-only in which case the cache mode 
> doesn't matter. But I'm not sure if this is enforced in libxl.

xl-disk-configuration.txt states the default 'access=' value for cdrom devices
is readonly. It doesn't mention readonly is enforced for cdrom devices, but
xlu_disk_parse() in libxlutil unconditionally sets the disk's readwrite field to
0 for cdroms.

> IMHO, we should make sure ro is enforced with cdrom devices and then we can 
> use the default cache mode.

I'll send a V2 based on the above findings.

Regards,
Jim

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH V2] libxl: don't add cache mode for qdisk cdrom drives

[Jim Fehlig]

qemu commit 91a097e7 forbids specifying cache mode for empty
drives. Attempting to create a domain with an empty qdisk cdrom
drive results in

qemu-system-x86_64: -drive if=ide,index=1,readonly=on,media=cdrom,
   cache=writeback,id=ide-832: Must specify either driver or file

libxl only allows an empty 'target=' for cdroms. By default, cdroms
are readonly (see the 'access' parameter in xl-disk-configuration.txt)
and forced to readonly by any tools (e.g. xl) using libxlutil's
xlu_disk_parse() function. With cdroms always marked readonly,
explicitly specifying the cache mode for cdrom drives can be dropped.
The drive's 'readonly=on' option can also be set unconditionally.

Signed-off-by: Jim Fehlig <jfeh...@suse.com>
---

V2:
Drop explicitly setting cache mode since cdrom devices are
readonly.
Unconditionally add 'readonly=on' drive option for cdroms.

 tools/libxl/libxl_dm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index fd12844..959e292 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1368,8 +1368,8 @@ static int libxl__build_device_model_args_new(libxl__gc 
*gc,
 
 if (disks[i].is_cdrom) {
 drive = libxl__sprintf(gc,
- 
"if=ide,index=%d,readonly=%s,media=cdrom,cache=writeback,id=ide-%i",
- disk, disks[i].readwrite ? "off" : "on", dev_number);
+ "if=ide,index=%d,readonly=on,media=cdrom,id=ide-%i",
+ disk, dev_number);
 
 if (target_path)
 drive = libxl__sprintf(gc, "%s,file=%s,format=%s",
-- 
1.8.0.1


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [libvirt test] 92667: regressions - FAIL

[Jim Fehlig]

On 04/27/2016 04:22 PM, Andrew Cooper wrote:
> On 27/04/2016 22:58, Jim Fehlig wrote:
>> On 04/25/2016 05:26 AM, osstest service owner wrote:
>>> flight 92667 libvirt real [real]
>>> http://logs.test-lab.xenproject.org/osstest/logs/92667/
>>>
>>> Regressions :-(
>>>
>>> Tests which did not succeed and are blocking,
>>> including tests which could not be run:
>>>  test-amd64-i386-libvirt  14 guest-saverestore fail REGR. vs. 
>>> 91479
>>>  test-amd64-amd64-libvirt-xsm 14 guest-saverestore fail REGR. vs. 
>>> 91479
>>>  test-amd64-amd64-libvirt-pair 21 guest-migrate/src_host/dst_host fail 
>>> REGR. vs. 91479
>>>  test-amd64-i386-libvirt-pair 21 guest-migrate/src_host/dst_host fail REGR. 
>>> vs. 91479
>>>  test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 12 guest-saverestore 
>>> fail REGR. vs. 91479
>>>  test-amd64-i386-libvirt-xsm  14 guest-saverestore fail REGR. vs. 
>>> 91479
>>>  test-amd64-amd64-libvirt-vhd 13 guest-saverestore fail REGR. vs. 
>>> 91479
>>>  test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 12 guest-saverestore 
>>> fail REGR. vs. 91479
>>>  test-amd64-amd64-libvirt 14 guest-saverestore fail REGR. vs. 
>>> 91479
>> All of these save/restore and migration failures show the following error on 
>> the
>> restore side
>>
>> 2016-04-25 10:16:18 UTC libxl: error:
>> libxl_exec.c:118:libxl_report_child_exitstatus: conversion helper [26771] 
>> exited
>> with error status 1
>> 2016-04-25 10:16:18 UTC libxl: error: libxl_utils.c:507:libxl_read_exactly:
>> file/stream truncated reading ipc msg header from domain 1 save/restore 
>> helper
>> stdout pipe
>> 2016-04-25 10:16:18 UTC libxl: error:
>> libxl_exec.c:129:libxl_report_child_exitstatus: domain 1 save/restore helper
>> [26772] died due to fatal signal Terminated
>>
>> I'm not sure if this problem has already been addressed by recent
>> migration-related fixes.
> This is testing two different versions of libvirt against the same
> version of libxl.
>
> Looking at
> http://logs.test-lab.xenproject.org/osstest/logs/92667/test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm/italia0---var-log-libvirt-libxl-libxl-driver.log
>
> 2016-04-25 08:36:03 UTC xc: progress: End of stream: 0/00%
>
> indicates that the save side is in v2 format (which is expected).  (I
> should add at least an info print in libxl_stream_write() indicating the
> pertinent  details).
>
> On the restore side,
>
> 2016-04-25 08:36:20 UTC libxl: debug:
> libxl_stream_read.c:358:stream_header_done: Stream v2 (from legacy)
> 2016-04-25 08:36:20 UTC libxl: debug:
> libxl_stream_read.c:574:process_record: Record: 1, length 0
> 2016-04-25 08:36:20 UTC libxl: error:
> libxl_exec.c:118:libxl_report_child_exitstatus: conversion helper [3909]
> exited with error status 1
>
> which means that the restore code was told that the stream was in legacy
> format.  The legacy conversion script was forked and found that the
> stream wasn't legacy.  (I have no idea where the real error message went
> from that - it should be plumbed through into a info message, and
> definitely does work when running `xl` on the command line).
>
> I suspect this is breakage from the LIBXL_ABI_VERSION changes.
>
> Because of the short-sightest mess that legacy migration was, it is not
> possible for libxl to distinguish a legacy stream from a v2 stream in
> libxl_domain_create_restore().  The caller (i.e. libvirt) must provide
> the correct stream version in libxl_domain_restore_params.

How do I handle the case of a libvirt+Xen migV2 host migrating a domain to a
libvirt+Xen migV1 host? Do you know how that scenario is handled in xl? Or is
migrating a domain from migV2 host to migV1 host not supported?

Thanks for  the help.

Regards,
Jim
>
> ~Andrew
>


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [libvirt test] 92667: regressions - FAIL

[Jim Fehlig]

On 04/25/2016 05:26 AM, osstest service owner wrote:
> flight 92667 libvirt real [real]
> http://logs.test-lab.xenproject.org/osstest/logs/92667/
>
> Regressions :-(
>
> Tests which did not succeed and are blocking,
> including tests which could not be run:
>  test-amd64-i386-libvirt  14 guest-saverestore fail REGR. vs. 
> 91479
>  test-amd64-amd64-libvirt-xsm 14 guest-saverestore fail REGR. vs. 
> 91479
>  test-amd64-amd64-libvirt-pair 21 guest-migrate/src_host/dst_host fail REGR. 
> vs. 91479
>  test-amd64-i386-libvirt-pair 21 guest-migrate/src_host/dst_host fail REGR. 
> vs. 91479
>  test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 12 guest-saverestore fail 
> REGR. vs. 91479
>  test-amd64-i386-libvirt-xsm  14 guest-saverestore fail REGR. vs. 
> 91479
>  test-amd64-amd64-libvirt-vhd 13 guest-saverestore fail REGR. vs. 
> 91479
>  test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 12 guest-saverestore fail 
> REGR. vs. 91479
>  test-amd64-amd64-libvirt 14 guest-saverestore fail REGR. vs. 
> 91479

All of these save/restore and migration failures show the following error on the
restore side

2016-04-25 10:16:18 UTC libxl: error:
libxl_exec.c:118:libxl_report_child_exitstatus: conversion helper [26771] exited
with error status 1
2016-04-25 10:16:18 UTC libxl: error: libxl_utils.c:507:libxl_read_exactly:
file/stream truncated reading ipc msg header from domain 1 save/restore helper
stdout pipe
2016-04-25 10:16:18 UTC libxl: error:
libxl_exec.c:129:libxl_report_child_exitstatus: domain 1 save/restore helper
[26772] died due to fatal signal Terminated

I'm not sure if this problem has already been addressed by recent
migration-related fixes.

Regards,
Jim


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] libxl: don't add cache mode for empty drives

[Jim Fehlig]

On 04/27/2016 01:38 AM, Roger Pau Monné wrote:
> On Tue, Apr 26, 2016 at 10:35:31PM -0600, Jim Fehlig wrote:
>> qemu commit 91a097e7 forbids specifying the cache mode for empty
>> drives. Attempting to create a domain with an empty qdisk cdrom
>> results in
>>
>> qemu-system-x86_64: -drive if=ide,index=1,readonly=on,media=cdrom,
>>cache=writeback,id=ide-832: Must specify either driver or file
>>
>> Change libxl to only emit cache mode when a cdrom target is specified.
> What happens then when a cdrom is inserted? I cannot seem to find the code 
> in libxl_cdrom_insert that sets the cache mode.

I cannot find it either. I suppose it would need to be setup via xenstore,
similar to other options like feature_discard. But looking at
$qemu-src/hw/block/xen_disk.c, it seems the XenBlkDev struct has no field to
specify cache mode. Would qemu's xen_disk need to be extended to support cache
mode, followed by a libxl patch to set the cache mode in xenstore?

>  Is the default one used 
> then?

Yes, the default cache mode (which is already writeback AIUI) would be used if
not explicitly specified. Which brings up the option of removing
'cache=writeback' for cdroms altogether. Any opinion on that option?

Regards,
Jim


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH] libxl: don't add cache mode for empty drives

[Jim Fehlig]

qemu commit 91a097e7 forbids specifying the cache mode for empty
drives. Attempting to create a domain with an empty qdisk cdrom
results in

qemu-system-x86_64: -drive if=ide,index=1,readonly=on,media=cdrom,
   cache=writeback,id=ide-832: Must specify either driver or file

Change libxl to only emit cache mode when a cdrom target is specified.

Signed-off-by: Jim Fehlig <jfeh...@suse.com>
---
 tools/libxl/libxl_dm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index fd12844..df1207b 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1368,11 +1368,11 @@ static int libxl__build_device_model_args_new(libxl__gc 
*gc,
 
 if (disks[i].is_cdrom) {
 drive = libxl__sprintf(gc,
- 
"if=ide,index=%d,readonly=%s,media=cdrom,cache=writeback,id=ide-%i",
+ "if=ide,index=%d,readonly=%s,media=cdrom,id=ide-%i",
  disk, disks[i].readwrite ? "off" : "on", dev_number);
 
 if (target_path)
-drive = libxl__sprintf(gc, "%s,file=%s,format=%s",
+drive = libxl__sprintf(gc, 
"%s,file=%s,format=%s,cache=writeback",
drive, target_path, format);
 } else {
 /*
-- 
2.1.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [libvirt test] 91597: regressions - FAIL

[Jim Fehlig]

On 04/16/2016 05:54 AM, osstest service owner wrote:
> flight 91597 libvirt real [real]
> http://logs.test-lab.xenproject.org/osstest/logs/91597/
>
> Regressions :-(
>
> Tests which did not succeed and are blocking,
> including tests which could not be run:
>  build-amd64-libvirt   5 libvirt-build fail REGR. vs. 
> 91479
>  build-i386-libvirt5 libvirt-build fail REGR. vs. 
> 91479
>  build-armhf-libvirt   5 libvirt-build fail REGR. vs. 
> 91479

These build failures (warnings treated as errors) surfaced after defining
LIBXL_API_VERSION=0x040200. E.g.

In file included from
/home/osstest/build.91597.build-amd64-libvirt/xendist/usr/local/include/libxl.h:439:0,
 from libxl/libxl_domain.h:27,
 from libxl/libxl_domain.c:28:
/home/osstest/build.91597.build-amd64-libvirt/xendist/usr/local/include/libxl_uuid.h:64:1:
error: 'static' is not at beginning of declaration 
[-Werror=old-style-declaration]
 void static inline libxl_uuid_copy_0x040400(libxl_uuid *dst,
 ^
/home/osstest/build.91597.build-amd64-libvirt/xendist/usr/local/include/libxl_uuid.h:64:1:
error: 'inline' is not at beginning of declaration 
[-Werror=old-style-declaration]

I suppose the obvious fix is to change the declarations and backport to all
maintained releases. Another option is suppressing the warning. Or perhaps a
better trick?

Regards,
Jim


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH libvirt v2 2/2] libxl: support vscsi

[Jim Fehlig]

On 04/13/2016 03:15 AM, Olaf Hering wrote:
> This uses the API version of the proposed vscsi support in libxl (v12):
> http://lists.xenproject.org/archives/html/xen-devel/2016-04/msg01772.html

We'll need to wait for that to land in xen.git before committing libxl scsi
support to libvirt.git, but thanks for putting the current work up for review.

> Is there anything else that needs to be done in libvirt?

We'll need code in src/xenconfig/xen_xl.* to convert xl vscis cfg <-> libvirt
domXML, along with tests in tests/xlconfigtest.

>  Right now libvirt scsi
> support is very simple minded, no support at all to describe host devices with
> persistant names.

Yes, I think you are correct, but I'm not aware of any reason that support
couldn't be added.

>  Example used during testing:
>
>   rawio='yes'>

IIRC, the 'managed' attribute only applies to PCI hostdevs.

>   
>
>
>   
>   
>   
>  
>
> Signed-off-by: Olaf Hering <o...@aepfle.de>
> Cc: Jim Fehlig <jfeh...@suse.com>
> ---
>  src/libxl/libxl_conf.c   |  59 +++
>  src/libxl/libxl_conf.h   |   1 +
>  src/libxl/libxl_domain.c |   2 +-
>  src/libxl/libxl_driver.c | 187 
> +++
>  4 files changed, 248 insertions(+), 1 deletion(-)

Needs rebased against current libvirt.git master.

>
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index f5ef50f..1e3615e 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -1915,6 +1915,61 @@ libxlMakePCIList(virDomainDefPtr def, 
> libxl_domain_config *d_config)
>  }
>  
>  static int
> +libxlMakeVscsiList(libxl_ctx *ctx,
> +   XLU_Config *xlu,
> +   virDomainDefPtr def,
> +   libxl_domain_config *d_config)
> +{
> +virDomainHostdevDefPtr *l_hostdevs = def->hostdevs;
> +size_t i, nhostdevs = def->nhostdevs;
> +virDomainHostdevDefPtr hostdev;
> +virDomainHostdevSubsysSCSIPtr scsisrc;
> +char *str;
> +int rc = 0;
> +
> +if (nhostdevs == 0)
> +return 0;
> +
> +for (i = 0; i < nhostdevs; i++) {
> +hostdev = l_hostdevs[i];
> +if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS)
> +continue;
> +if (hostdev->source.subsys.type != 
> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI)
> +continue;
> +scsisrc = >source.subsys.u.scsi;
> +if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI)
> +continue;
> +#if defined(LIBXL_HAVE_VSCSI)
> +if (virAsprintf(, "%s:%u:%u:%u,%u:%u:%u:%llu%s",
> + scsisrc->u.host.adapter + strlen("scsi_host"),
> + scsisrc->u.host.bus,
> + scsisrc->u.host.target,
> + scsisrc->u.host.unit,
> + hostdev->info->addr.drive.controller,
> + hostdev->info->addr.drive.bus,
> + hostdev->info->addr.drive.target,
> + hostdev->info->addr.drive.unit,
> + scsisrc->rawio == VIR_TRISTATE_BOOL_YES ? 
> ",feature-host" : "") < 0) {
> +goto error;
> +};
> +rc = xlu_vscsi_config_add(xlu, ctx, str, _config->num_vscsictrls, 
> _config->vscsictrls);
> +VIR_FREE(str);
> +if (rc)
> +goto error;
> +#else
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +   _("This version of libxenlight does not support 
> vscsi"));
> +goto error;
> +#endif
> +}
> +
> +return 0;
> +
> + error:
> +return -1;
> +}
> +
> +static int
>  libxlMakeVideo(virDomainDefPtr def, libxl_domain_config *d_config)
>  
>  {
> @@ -2059,6 +2114,7 @@ int
>  libxlBuildDomainConfig(virPortAllocatorPtr graphicsports,
> virDomainDefPtr def,
> libxl_ctx *ctx,
> +   XLU_Config *xlu,
> libxl_domain_config *d_config)
>  {
>  libxl_domain_config_init(d_config);
> @@ -2084,6 +2140,9 @@ libxlBuildDomainConfig(virPortAllocatorPtr 
> graphicsports,
>  if (libxlMakePCIList(def, d_config) < 0)
>  return -1;
>  
> +if (libxlMakeVscsiList(ctx, xlu, def, d_config) < 0)
> +return -1;
> +
>  /*
>   * Now that any potential VFBs are defined, update the build info with
>   * the data of the primary display. Some day libxl might implicitely do
> diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h
>

Re: [Xen-devel] [PATCH libvirt v2 1/2] libxl: include a XLU_Config in _libxlDriverConfig

[Jim Fehlig]

On 04/13/2016 03:15 AM, Olaf Hering wrote:
> Upcoming changes for vscsi will use libxlutil.so to prepare the
> configuration for libxl. The helpers needs a xlu struct for logging.
> Provide one and reuse the existing output as log target.
>
> Signed-off-by: Olaf Hering <o...@aepfle.de>
> Cc: Jim Fehlig <jfeh...@suse.com>
> ---
>  src/libxl/libxl_conf.c | 7 +++
>  src/libxl/libxl_conf.h | 7 +++
>  2 files changed, 14 insertions(+)
>
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index d16280d..f5ef50f 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -93,6 +93,7 @@ libxlDriverConfigDispose(void *obj)
>  virObjectUnref(cfg->caps);
>  libxl_ctx_free(cfg->ctx);
>  xtl_logger_destroy(cfg->logger);
> +xlu_cfg_destroy(cfg->xlu);

This fails to compile if HAVE_LIBXLUTIL_H is not defined.

>  if (cfg->logger_file)
>  VIR_FORCE_FCLOSE(cfg->logger_file);
>  
> @@ -1738,6 +1739,12 @@ libxlDriverConfigNew(void)
>  goto error;
>  }
>  
> +cfg->xlu = xlu_cfg_init(cfg->logger_file, "libvirt");
> +if (!cfg->xlu) {
> +VIR_ERROR(_("cannot create xlu for libxenlight, disabling driver"));
> +goto error;
> +}
> +
>  if (libxl_ctx_alloc(>ctx, LIBXL_VERSION, 0, cfg->logger)) {
>  VIR_ERROR(_("cannot initialize libxenlight context, probably not "
>  "running in a Xen Dom0, disabling driver"));
> diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h
> index 3c0eafb..b069e45 100644
> --- a/src/libxl/libxl_conf.h
> +++ b/src/libxl/libxl_conf.h
> @@ -27,6 +27,12 @@
>  # define LIBXL_CONF_H
>  
>  # include 
> +# ifdef HAVE_LIBXLUTIL_H
> +#  include 
> +# else
> +typedef struct XLU_Config XLU_Config;
> +XLU_Config *xlu_cfg_init(FILE *report, const char *report_filename);

You'll need to add xlu_cfg_destroy here.

Regards,
Jim

> +# endif
>  
>  # include "internal.h"
>  # include "libvirt_internal.h"
> @@ -96,6 +102,7 @@ struct _libxlDriverConfig {
>  /* log stream for driver-wide libxl ctx */
>  FILE *logger_file;
>  xentoollog_logger *logger;
> +XLU_Config *xlu;
>  /* libxl ctx for driver wide ops; getVersion, getNodeInfo, ... */
>  libxl_ctx *ctx;
>  
>
>
>


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [libvirt] [PATCH V2] libxl: use LIBXL_API_VERSION 0x040200

[Jim Fehlig]

On 04/15/2016 03:48 AM, Martin Kletzander wrote:
> On Thu, Apr 14, 2016 at 04:35:12PM -0600, Jim Fehlig wrote:
>> To ensure the libvirt libxl driver will build with future versions
>> of Xen where the libxl API may change in incompatible ways,
>> explicitly use LIBXL_API_VERSION 0x040200. The libxl driver
>> does use new libxl APIs that have been added since Xen 4.2, but
>> currently it does not make use of any changes made to existing
>> APIs such as libxl_domain_create_restore or libxl_set_vcpuaffinity.
>> The version can be bumped if/when the libxl driver consumes the
>> changed APIs.
>>
>> Further details can be found in the following discussion thread
>>
>> https://www.redhat.com/archives/libvir-list/2016-April/msg00178.html
>> Signed-off-by: Jim Fehlig <jfeh...@suse.com>
>> ---
>> configure.ac |  2 ++
>> src/libxl/libxl_conf.h   | 12 
>> src/libxl/libxl_domain.c | 15 ---
>> 3 files changed, 2 insertions(+), 27 deletions(-)
>>
>> diff --git a/configure.ac b/configure.ac
>> index b1500f6..446f2a2 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -894,6 +894,7 @@ if test "$with_libxl" != "no" ; then
>> PKG_CHECK_MODULES([LIBXL], [xenlight], [
>>  LIBXL_FIRMWARE_DIR=`$PKG_CONFIG --variable xenfirmwaredir xenlight`
>>  LIBXL_EXECBIN_DIR=`$PKG_CONFIG --variable libexec_bin xenlight`
>> + LIBXL_CFLAGS="$LIBXL_CFLAGS -DLIBXL_API_VERSION=0x040200"
>>  with_libxl=yes
>> ], [LIBXL_FOUND=no])
>> if test "$LIBXL_FOUND" = "no"; then
>> @@ -906,6 +907,7 @@ if test "$with_libxl" != "no" ; then
>> LIBS="$LIBS $LIBXL_LIBS"
>> AC_CHECK_LIB([xenlight], [libxl_ctx_alloc], [
>> with_libxl=yes
>> +LIBXL_CFLAGS="$LIBXL_CFLAGS -DLIBXL_API_VERSION=0x040200"
>> LIBXL_LIBS="$LIBXL_LIBS -lxenlight"
>> ],[
>> if test "$with_libxl" = "yes"; then
>
>
> Looks good, I'd just adjust these two hunks so that it's added in one
> place (and can be changed in one place later on so we don't fall into
> trouble accidentally.  ACK with that changed.

Thanks. I used one LIBXL_CFLAGS="$LIBXL_CFLAGS -DLIBXL_API_VERSION=0x040200"
after the test for libxenlight, and added a comment for good measure. Pushed 
now.

Regards,
Jim


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [libvirt] [PATCH] libxl: use LIBXL_API_VERSION 0x040200

[Jim Fehlig]

Martin Kletzander wrote:
> On Wed, Apr 13, 2016 at 05:37:05PM -0600, Jim Fehlig wrote:
>> To ensure the libvirt libxl driver will build with future versions
>> of Xen where the libxl API may change in incompatible ways,
>> explicitly use LIBXL_API_VERSION 0x040200. The libxl driver
>> does use new libxl APIs that have been added since Xen 4.2, but
>> currently it does not make use of any changes made to existing
>> APIs such as libxl_domain_create_restore or libxl_set_vcpuaffinity.
>> The version can be bumped if/when the libxl driver consumes the
>> changed APIs.
>>
>> Further details can be found in the following discussion thread
>>
>> https://www.redhat.com/archives/libvir-list/2016-April/msg00178.html
>> Signed-off-by: Jim Fehlig <jfeh...@suse.com>
>> ---
>> src/Makefile.am  |  1 +
>> src/libxl/libxl_conf.h   | 12 
>> src/libxl/libxl_domain.c | 15 ---
>> 3 files changed, 1 insertion(+), 27 deletions(-)
>>
>> diff --git a/src/Makefile.am b/src/Makefile.am
>> index 08ff301..259a474 100644
>> --- a/src/Makefile.am
>> +++ b/src/Makefile.am
>> @@ -1311,6 +1311,7 @@ endif ! WITH_DRIVER_MODULES
>>
>> libvirt_driver_libxl_impl_la_CFLAGS = \
>> $(LIBXL_CFLAGS)\
>> +-DLIBXL_API_VERSION=0x040200\
> 
> Adding it to LIBXL_CFLAGS in configure.ac would make it show after
> configure, but that's something probably only I would find interesting
> =)

I think that is a good idea. I've sent a V2 that moves the define from
src/Makefile.am to configure.ac

https://www.redhat.com/archives/libvir-list/2016-April/msg00938.html

> I'm no libxl expert and I don't have it on my system, so I can't try
> building it, hence only weak ACK from me.  If it build for you then it's
> fine as nobody objects as I see.

I've built libvirt with the V2 patch against Xen 4.2, 4.3, 4.4, and 4.7/master.

Regards,
Jim

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] libxl: use LIBXL_API_VERSION 0x040200

[Jim Fehlig]

George Dunlap wrote:
> On Thu, Apr 14, 2016 at 12:37 AM, Jim Fehlig <jfeh...@suse.com> wrote:
>> To ensure the libvirt libxl driver will build with future versions
>> of Xen where the libxl API may change in incompatible ways,
>> explicitly use LIBXL_API_VERSION 0x040200. The libxl driver
>> does use new libxl APIs that have been added since Xen 4.2, but
>> currently it does not make use of any changes made to existing
>> APIs such as libxl_domain_create_restore or libxl_set_vcpuaffinity.
>> The version can be bumped if/when the libxl driver consumes the
>> changed APIs.
>>
>> Further details can be found in the following discussion thread
>>
>> https://www.redhat.com/archives/libvir-list/2016-April/msg00178.html
>> Signed-off-by: Jim Fehlig <jfeh...@suse.com>
>> ---
>>  src/Makefile.am  |  1 +
>>  src/libxl/libxl_conf.h   | 12 
>>  src/libxl/libxl_domain.c | 15 ---
>>  3 files changed, 1 insertion(+), 27 deletions(-)
>>
>> diff --git a/src/Makefile.am b/src/Makefile.am
>> index 08ff301..259a474 100644
>> --- a/src/Makefile.am
>> +++ b/src/Makefile.am
>> @@ -1311,6 +1311,7 @@ endif ! WITH_DRIVER_MODULES
>>
>>  libvirt_driver_libxl_impl_la_CFLAGS =  \
>> $(LIBXL_CFLAGS) \
>> +   -DLIBXL_API_VERSION=0x040200\
>> -I$(srcdir)/access  \
>> -I$(srcdir)/conf\
>> -I$(srcdir)/secret  \
>> diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h
>> index 3c0eafb..24e2911 100644
>> --- a/src/libxl/libxl_conf.h
>> +++ b/src/libxl/libxl_conf.h
>> @@ -69,18 +69,6 @@
>>  # endif
>>
>>
>> -/* libxl interface for setting VCPU affinity changed in 4.5. In fact, a new
>> - * parameter has been added, representative of 'VCPU soft affinity'. If one
>> - * does not care about it (and that's libvirt case), passing NULL is the
>> - * right thing to do. To mark that change, LIBXL_HAVE_VCPUINFO_SOFT_AFFINITY
>> - * is defined. */
>> -# ifdef LIBXL_HAVE_VCPUINFO_SOFT_AFFINITY
>> -#  define libxl_set_vcpuaffinity(ctx, domid, vcpuid, map) \
>> -libxl_set_vcpuaffinity((ctx), (domid), (vcpuid), (map), NULL)
>> -#  define libxl_set_vcpuaffinity_all(ctx, domid, max_vcpus, map) \
>> -libxl_set_vcpuaffinity_all((ctx), (domid), (max_vcpus), (map), NULL)
>> -# endif
> 
> Just checking with this -- the LIBXL_API_VERSION is meant for
> *forward* compatibility, but I think the LIBXL_HAVE_* macros are for
> *backwards* compatibility, isn't that right?  If you compile this
> against an older version of Xen without LIBXL_HAVE_VCPU_SOFT_AFFINITY
> (say, 4.4), will it break with this patch?

I've tested the patch (well, actually a variant that moves
'-DLIBXL_API_VERSION=0x040200' to configure.ac as suggested by Martin) by
building against 4.2, 4.3, 4.4, and 4.7/master. I think Dario already did a good
job explaining the removal of the above hunk.

Regards,
Jim

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH V2] libxl: use LIBXL_API_VERSION 0x040200

[Jim Fehlig]

To ensure the libvirt libxl driver will build with future versions
of Xen where the libxl API may change in incompatible ways,
explicitly use LIBXL_API_VERSION 0x040200. The libxl driver
does use new libxl APIs that have been added since Xen 4.2, but
currently it does not make use of any changes made to existing
APIs such as libxl_domain_create_restore or libxl_set_vcpuaffinity.
The version can be bumped if/when the libxl driver consumes the
changed APIs.

Further details can be found in the following discussion thread

https://www.redhat.com/archives/libvir-list/2016-April/msg00178.html
Signed-off-by: Jim Fehlig <jfeh...@suse.com>
---
 configure.ac |  2 ++
 src/libxl/libxl_conf.h   | 12 
 src/libxl/libxl_domain.c | 15 ---
 3 files changed, 2 insertions(+), 27 deletions(-)

diff --git a/configure.ac b/configure.ac
index b1500f6..446f2a2 100644
--- a/configure.ac
+++ b/configure.ac
@@ -894,6 +894,7 @@ if test "$with_libxl" != "no" ; then
 PKG_CHECK_MODULES([LIBXL], [xenlight], [
  LIBXL_FIRMWARE_DIR=`$PKG_CONFIG --variable xenfirmwaredir xenlight`
  LIBXL_EXECBIN_DIR=`$PKG_CONFIG --variable libexec_bin xenlight`
+ LIBXL_CFLAGS="$LIBXL_CFLAGS -DLIBXL_API_VERSION=0x040200"
  with_libxl=yes
 ], [LIBXL_FOUND=no])
 if test "$LIBXL_FOUND" = "no"; then
@@ -906,6 +907,7 @@ if test "$with_libxl" != "no" ; then
 LIBS="$LIBS $LIBXL_LIBS"
 AC_CHECK_LIB([xenlight], [libxl_ctx_alloc], [
 with_libxl=yes
+LIBXL_CFLAGS="$LIBXL_CFLAGS -DLIBXL_API_VERSION=0x040200"
 LIBXL_LIBS="$LIBXL_LIBS -lxenlight"
 ],[
 if test "$with_libxl" = "yes"; then
diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h
index 3c0eafb..24e2911 100644
--- a/src/libxl/libxl_conf.h
+++ b/src/libxl/libxl_conf.h
@@ -69,18 +69,6 @@
 # endif
 
 
-/* libxl interface for setting VCPU affinity changed in 4.5. In fact, a new
- * parameter has been added, representative of 'VCPU soft affinity'. If one
- * does not care about it (and that's libvirt case), passing NULL is the
- * right thing to do. To mark that change, LIBXL_HAVE_VCPUINFO_SOFT_AFFINITY
- * is defined. */
-# ifdef LIBXL_HAVE_VCPUINFO_SOFT_AFFINITY
-#  define libxl_set_vcpuaffinity(ctx, domid, vcpuid, map) \
-libxl_set_vcpuaffinity((ctx), (domid), (vcpuid), (map), NULL)
-#  define libxl_set_vcpuaffinity_all(ctx, domid, max_vcpus, map) \
-libxl_set_vcpuaffinity_all((ctx), (domid), (max_vcpus), (map), NULL)
-# endif
-
 typedef struct _libxlDriverPrivate libxlDriverPrivate;
 typedef libxlDriverPrivate *libxlDriverPrivatePtr;
 
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index 86fb713..14a900c 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -1026,9 +1026,6 @@ libxlDomainStart(libxlDriverPrivatePtr driver, 
virDomainObjPtr vm,
 int managed_save_fd = -1;
 libxlDomainObjPrivatePtr priv = vm->privateData;
 libxlDriverConfigPtr cfg;
-#ifdef LIBXL_HAVE_DOMAIN_CREATE_RESTORE_PARAMS
-libxl_domain_restore_params params;
-#endif
 virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr;
 libxl_asyncprogress_how aop_console_how;
 
@@ -1118,20 +1115,8 @@ libxlDomainStart(libxlDriverPrivatePtr driver, 
virDomainObjPtr vm,
 ret = libxl_domain_create_new(cfg->ctx, _config,
   , NULL, _console_how);
 } else {
-#if defined(LIBXL_HAVE_DOMAIN_CREATE_RESTORE_SEND_BACK_FD)
-params.checkpointed_stream = 0;
-ret = libxl_domain_create_restore(cfg->ctx, _config, ,
-  restore_fd, -1, , NULL,
-  _console_how);
-#elif defined(LIBXL_HAVE_DOMAIN_CREATE_RESTORE_PARAMS)
-params.checkpointed_stream = 0;
-ret = libxl_domain_create_restore(cfg->ctx, _config, ,
-  restore_fd, , NULL,
-  _console_how);
-#else
 ret = libxl_domain_create_restore(cfg->ctx, _config, ,
   restore_fd, NULL, _console_how);
-#endif
 }
 virObjectLock(vm);
 
-- 
2.6.1


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [libvirt] Fixing libvirt's libxl driver breakage -- where to define LIBXL_API_VERSION?

[Jim Fehlig]

On 04/13/2016 03:26 AM, Daniel P. Berrange wrote:
> On Wed, Apr 13, 2016 at 10:09:16AM +0100, George Dunlap wrote:
>> On Tue, Apr 12, 2016 at 10:31 PM, Jim Fehlig <jfeh...@suse.com> wrote:
>>> Wei Liu wrote:
>>>> Hi libvirt maintainers,
>>> Sorry for the delay. Slowly catching up on mail after vacation...
>>>
>>>> Xen's control library libxenlight (libxl) requires application
>>>> (libvirt in this case) to explictily define LIBXL_API_VERSION.
>>> Where is this requirement written? :-)
>>>
>>>> This is
>>>> lacking at the moment so libvirt's libxl driver always gets the latest
>>>> APIs.
>>> IMO, that is what we want for upstream libvirt. Downstreams can choose a
>>> specific version if they want.
>> I think one of us isn't understanding the situation properly. Is it
>> not the case that currently, all releases of libvirt *will not
>> compile* against Xen 4.7 once it's released?  So people downloading
>> and building libvirt will have to either 1) root around and try to
>> figure out what version of Xen it will build against, 2) manually add
>> in a #define *with the correct API version* to a header somewhere to
>> make it build properly, or 3) update to a version of libvirt that
>> supports the new api (which at the moment hasn't even been released
>> yet)?
>>
>> All of those options are completely unacceptable.  Older versions of
>> libvirt should Just Work when compiled against newer versions of Xen.
>>
>> I think it does make sense to have the libvirt development branch not
>> specify an API version; but when it branches for release, it should
>> set LIBXL_API_VERSION to whatever the current version is at the time
>> of the branch.
> FYI, libvirt doesn't do branching for releases - we always just cut the
> release straight from the master branch. We only actually create branches
> on demand, when we find we want to backport fixes to a previous release.
>
> Does libvirt master really need to always use the latest API version ?
>
> It feels like libvirt could just set LIBXL_API_VERSION to the lowest
> version it requires in order to get the functionality we know we are
> able to currently build against.

I think that is a good idea too. I've sent a patch setting LIBXL_API_VERSION to
0x040200

https://www.redhat.com/archives/libvir-list/2016-April/msg00771.html

Changes made to the APIs since 0x040200 (params added to
libxl_domain_create_restore and libxl_set_vcpuaffinity) are currently not used
by libvirt.

Regards,
Jim


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH] libxl: use LIBXL_API_VERSION 0x040200

[Jim Fehlig]

To ensure the libvirt libxl driver will build with future versions
of Xen where the libxl API may change in incompatible ways,
explicitly use LIBXL_API_VERSION 0x040200. The libxl driver
does use new libxl APIs that have been added since Xen 4.2, but
currently it does not make use of any changes made to existing
APIs such as libxl_domain_create_restore or libxl_set_vcpuaffinity.
The version can be bumped if/when the libxl driver consumes the
changed APIs.

Further details can be found in the following discussion thread

https://www.redhat.com/archives/libvir-list/2016-April/msg00178.html
Signed-off-by: Jim Fehlig <jfeh...@suse.com>
---
 src/Makefile.am  |  1 +
 src/libxl/libxl_conf.h   | 12 
 src/libxl/libxl_domain.c | 15 ---
 3 files changed, 1 insertion(+), 27 deletions(-)

diff --git a/src/Makefile.am b/src/Makefile.am
index 08ff301..259a474 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -1311,6 +1311,7 @@ endif ! WITH_DRIVER_MODULES
 
 libvirt_driver_libxl_impl_la_CFLAGS =  \
$(LIBXL_CFLAGS) \
+   -DLIBXL_API_VERSION=0x040200\
-I$(srcdir)/access  \
-I$(srcdir)/conf\
-I$(srcdir)/secret  \
diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h
index 3c0eafb..24e2911 100644
--- a/src/libxl/libxl_conf.h
+++ b/src/libxl/libxl_conf.h
@@ -69,18 +69,6 @@
 # endif
 
 
-/* libxl interface for setting VCPU affinity changed in 4.5. In fact, a new
- * parameter has been added, representative of 'VCPU soft affinity'. If one
- * does not care about it (and that's libvirt case), passing NULL is the
- * right thing to do. To mark that change, LIBXL_HAVE_VCPUINFO_SOFT_AFFINITY
- * is defined. */
-# ifdef LIBXL_HAVE_VCPUINFO_SOFT_AFFINITY
-#  define libxl_set_vcpuaffinity(ctx, domid, vcpuid, map) \
-libxl_set_vcpuaffinity((ctx), (domid), (vcpuid), (map), NULL)
-#  define libxl_set_vcpuaffinity_all(ctx, domid, max_vcpus, map) \
-libxl_set_vcpuaffinity_all((ctx), (domid), (max_vcpus), (map), NULL)
-# endif
-
 typedef struct _libxlDriverPrivate libxlDriverPrivate;
 typedef libxlDriverPrivate *libxlDriverPrivatePtr;
 
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index aed904b..192a506 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -981,9 +981,6 @@ libxlDomainStart(libxlDriverPrivatePtr driver, 
virDomainObjPtr vm,
 int managed_save_fd = -1;
 libxlDomainObjPrivatePtr priv = vm->privateData;
 libxlDriverConfigPtr cfg;
-#ifdef LIBXL_HAVE_DOMAIN_CREATE_RESTORE_PARAMS
-libxl_domain_restore_params params;
-#endif
 virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr;
 libxl_asyncprogress_how aop_console_how;
 
@@ -1070,20 +1067,8 @@ libxlDomainStart(libxlDriverPrivatePtr driver, 
virDomainObjPtr vm,
 ret = libxl_domain_create_new(cfg->ctx, _config,
   , NULL, _console_how);
 } else {
-#if defined(LIBXL_HAVE_DOMAIN_CREATE_RESTORE_SEND_BACK_FD)
-params.checkpointed_stream = 0;
-ret = libxl_domain_create_restore(cfg->ctx, _config, ,
-  restore_fd, -1, , NULL,
-  _console_how);
-#elif defined(LIBXL_HAVE_DOMAIN_CREATE_RESTORE_PARAMS)
-params.checkpointed_stream = 0;
-ret = libxl_domain_create_restore(cfg->ctx, _config, ,
-  restore_fd, , NULL,
-  _console_how);
-#else
 ret = libxl_domain_create_restore(cfg->ctx, _config, ,
   restore_fd, NULL, _console_how);
-#endif
 }
 virObjectLock(vm);
 
-- 
2.1.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] Fixing libvirt's libxl driver breakage -- where to define LIBXL_API_VERSION?

[Jim Fehlig]

On 04/13/2016 03:09 AM, George Dunlap wrote:
> On Tue, Apr 12, 2016 at 10:31 PM, Jim Fehlig <jfeh...@suse.com> wrote:
>> Wei Liu wrote:
>>> Hi libvirt maintainers,
>> Sorry for the delay. Slowly catching up on mail after vacation...
>>
>>> Xen's control library libxenlight (libxl) requires application
>>> (libvirt in this case) to explictily define LIBXL_API_VERSION.
>> Where is this requirement written? :-)
>>
>>> This is
>>> lacking at the moment so libvirt's libxl driver always gets the latest
>>> APIs.
>> IMO, that is what we want for upstream libvirt. Downstreams can choose a
>> specific version if they want.
> I think one of us isn't understanding the situation properly. Is it
> not the case that currently, all releases of libvirt *will not
> compile* against Xen 4.7 once it's released?  So people downloading
> and building libvirt will have to either 1) root around and try to
> figure out what version of Xen it will build against, 2) manually add
> in a #define *with the correct API version* to a header somewhere to
> make it build properly, or 3) update to a version of libvirt that
> supports the new api (which at the moment hasn't even been released
> yet)?
>
> All of those options are completely unacceptable.  Older versions of
> libvirt should Just Work when compiled against newer versions of Xen.

Yes, agreed. In practice though folks want a new libvirt with the new Xen, which
is probably why no one has complained thus far.

I'll knock up a patch to set the LIBXL_API_VERSION to 0x040200. The only APIs
that have changed since 0x040200 are libxl_set_vcpuaffinity and
libxl_domain_create_restore, but libvirt does not use the changes (added
params). libvirt does use new libxl APIs added since Xen 4.2, but those aren't
tied to a specific LIBXL_API_VERSION.

Regards,
Jim


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] Fixing libvirt's libxl driver breakage -- where to define LIBXL_API_VERSION?

[Jim Fehlig]

Wei Liu wrote:
> Hi libvirt maintainers,

Sorry for the delay. Slowly catching up on mail after vacation...

> 
> Xen's control library libxenlight (libxl) requires application
> (libvirt in this case) to explictily define LIBXL_API_VERSION.

Where is this requirement written? :-)

> This is
> lacking at the moment so libvirt's libxl driver always gets the latest
> APIs.

IMO, that is what we want for upstream libvirt. Downstreams can choose a
specific version if they want.

> We changed one of the APIs in libxl so libvirt's libxl driver
> can't build at the moment.

A quick glance ahead in my libvirt mail tells me you fixed this with the
preferred LIBXL_HAVE_* pattern.

Regards,
Jim

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v4 00/14] Load BIOS via toolstack instead of been embedded in hvmloader.

[Jim Fehlig]

On 04/01/2016 03:12 AM, George Dunlap wrote:
> So no, you haven't gotten rid of me just yet; on the contrary, you'll
> soon get 25% more. :-)

Looking forward to it :-).

Regards,
Jim


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v4 00/14] Load BIOS via toolstack instead of been embedded in hvmloader.

[Jim Fehlig]

Jan Beulich wrote:
 On 30.03.16 at 19:22,  wrote:
>> Anthony PERARD wrote:
>>> Hi all,
>>>
>>> Few changes in V4:
>>>   I leave the ACPI alone and only load the BIOS via libxl now.
>>>
>>>   Detail of changes in each patches.
>>>
>>> I've look at loading the BIOS via the toolstack instead of having them 
>> embedded
>>> in the hvmloader binary. After this patch series, hvmloader compilation 
>> would
>>> be indenpendant from SeaBIOS and OVMF compilation.
>> Hi Anthony, Wei,
>>
>> Any chance this series will make 4.7? AFAICT, there were no major issues in 
>> the
>> reviews. I didn't review all the patches in detail, but have done quite a 
>> bit of
>> testing with this series and haven't noticed any problems (beyond the 
>> current
>> upstream ovmf breakage with Xen).
> 
> I'm afraid the primary issue here is reviewing bandwidth: While
> the hvmloader parts are on my list of things to look at

And these are precisely the patches I just skimmed over, since I'm not familiar
with hvmloader.

Jim

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v4 00/14] Load BIOS via toolstack instead of been embedded in hvmloader.

[Jim Fehlig]

Anthony PERARD wrote:
> Hi all,
> 
> Few changes in V4:
>   I leave the ACPI alone and only load the BIOS via libxl now.
> 
>   Detail of changes in each patches.
> 
> I've look at loading the BIOS via the toolstack instead of having them 
> embedded
> in the hvmloader binary. After this patch series, hvmloader compilation would
> be indenpendant from SeaBIOS and OVMF compilation.

Hi Anthony, Wei,

Any chance this series will make 4.7? AFAICT, there were no major issues in the
reviews. I didn't review all the patches in detail, but have done quite a bit of
testing with this series and haven't noticed any problems (beyond the current
upstream ovmf breakage with Xen).

Regards,
Jim

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v4 00/14] Load BIOS via toolstack instead of been embedded in hvmloader.

[Jim Fehlig]

Anthony PERARD wrote:
> Hi all,
> 
> Few changes in V4:
>   I leave the ACPI alone and only load the BIOS via libxl now.
> 
>   Detail of changes in each patches.
> 
> I've look at loading the BIOS via the toolstack instead of having them 
> embedded
> in the hvmloader binary. After this patch series, hvmloader compilation would
> be indenpendant from SeaBIOS and OVMF compilation.
> 
> Here is a general view of the few step to load the BIOS:
> - libxl load the BIOS blob into it's memory and add it to struct
>   xc_hvm_build_args.bios_module
> - libxc load the blob into the guest memory and fill the struct
>   hvm_start_info and store a name for each module into the module cmdline.
> - hvmloader read the addresses from the hvm_start_info, find out which
>   module to load and copy the blob to the right place.
> 
> A git tree can be found here:
> git://xenbits.xen.org/people/aperard/xen-unstable.git
> tag: hvmloader-with-separated-bios-v4
> 
> Cc: Ian Jackson <ian.jack...@eu.citrix.com>
> Cc: Stefano Stabellini <stefano.stabell...@eu.citrix.com>
> Cc: Wei Liu <wei.l...@citrix.com>
> Cc: Andrew Cooper <andrew.coop...@citrix.com>
> Cc: Jan Beulich <jbeul...@suse.com>

I've been testing this series in conjunction with my downstream work to use
distro-provided qemu, SeaBIOS, and OVMF instead of the Xen built ones. I've
tested many combinations of these components, e.g. distro qemu and SeaBIOS,
qemu-xen with distro SeaBIOS, distro qemu with Xen SeaBIOS, etc., and haven't
noticed any problems. Therefore...

  Tested-by: Jim Fehlig <jfeh...@suse.com>

Regards,
Jim

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [ovmf test] 87014: regressions - FAIL

[Jim Fehlig]

Wei Liu wrote:
> On Wed, Mar 23, 2016 at 06:21:58PM -0600, Jim Fehlig wrote:
>> On 03/23/2016 09:27 AM, osstest service owner wrote:
>>> flight 87014 ovmf real [real]
>>> http://logs.test-lab.xenproject.org/osstest/logs/87014/
>>>
>>> Regressions :-(
>>>
>>> Tests which did not succeed and are blocking,
>>> including tests which could not be run:
>>>  test-amd64-amd64-xl-qemuu-ovmf-amd64 9 debian-hvm-install fail REGR. vs. 
>>> 65543
>>>  test-amd64-i386-xl-qemuu-ovmf-amd64  9 debian-hvm-install fail REGR. vs. 
>>> 65543
>> I've been testing Anthony's series to 'Load BIOS via toolstack instead of 
>> been
>> embedded in hvmloader' [0] in an attempt to use the distro ovmf instead of
>> "ovmf-xen". openSUSE Factory's ovmf package [1] is updated regularly with an
>> upstream git snapshot and I noticed VMs did not boot when using it. I 
>> bisected
>> and found ovmf commit 7b0a1ead the culprit
>>
>> commit 7b0a1ead7d2efa7f9eae4c2b254ff154d9c5f74f
>> Author: Ruiyu Ni <ruiyu...@intel.com>
>> Date:   Wed Feb 17 18:06:36 2016 +0800
>>
>> MdeModuelPkg/PciBus: Return AddrTranslationOffset in GetBarAttributes
>>
>> Some platform doesn't use CPU(HOST)/Device 1:1 mapping for PCI Bus.
>> But PCI IO doesn't have interface to tell caller (device driver)
>> whether the address returned by GetBarAttributes() is HOST address
>> or device address.
>> UEFI Spec 2.6 addresses this issue by clarifying the address returned
>> is HOST address and caller can use AddrTranslationOffset to calculate
>> the device address.
>>  
>> I'm sure it's biting this test too.
> 
> Yes, and OSSTest already fingered that commit. According to Anthony OVMF
> is even more broken now because more bugs crept in.
> 
> Anthony is working on fixing OVMF.

Cool. Thanks Anthony!

> 
>> BTW, any pointers on getting debug info from
>> hvmloader+ovmf+qemu+other-components-involved? With the broken ovmf, I got
>> nothing on the VM serial port, an empty, black vfb, nothing beyond "Invoking
>> OVMF..." in xl dmesg, and no errors in the qemu log file. Beyond bisecting, I
>> wasn't sure how to debug :-). I tried adding
>>
>> device_model_args=[ "-debugcon file:debug.log", "-global 
>> isa-debugcon.iobase=0x402"]
>>
> 
> Can you try
> 
>   device_model_args = ["-debugcon", "file:debug.log", "-global", 
> "isa-debugcon.iobase=0x402"]

Yes, that worked. Thanks.

Regards,
Jim

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [ovmf test] 87014: regressions - FAIL

[Jim Fehlig]

On 03/23/2016 09:27 AM, osstest service owner wrote:
> flight 87014 ovmf real [real]
> http://logs.test-lab.xenproject.org/osstest/logs/87014/
>
> Regressions :-(
>
> Tests which did not succeed and are blocking,
> including tests which could not be run:
>  test-amd64-amd64-xl-qemuu-ovmf-amd64 9 debian-hvm-install fail REGR. vs. 
> 65543
>  test-amd64-i386-xl-qemuu-ovmf-amd64  9 debian-hvm-install fail REGR. vs. 
> 65543

I've been testing Anthony's series to 'Load BIOS via toolstack instead of been
embedded in hvmloader' [0] in an attempt to use the distro ovmf instead of
"ovmf-xen". openSUSE Factory's ovmf package [1] is updated regularly with an
upstream git snapshot and I noticed VMs did not boot when using it. I bisected
and found ovmf commit 7b0a1ead the culprit

commit 7b0a1ead7d2efa7f9eae4c2b254ff154d9c5f74f
Author: Ruiyu Ni 
Date:   Wed Feb 17 18:06:36 2016 +0800

MdeModuelPkg/PciBus: Return AddrTranslationOffset in GetBarAttributes
   
Some platform doesn't use CPU(HOST)/Device 1:1 mapping for PCI Bus.
But PCI IO doesn't have interface to tell caller (device driver)
whether the address returned by GetBarAttributes() is HOST address
or device address.
UEFI Spec 2.6 addresses this issue by clarifying the address returned
is HOST address and caller can use AddrTranslationOffset to calculate
the device address.
 
I'm sure it's biting this test too.

BTW, any pointers on getting debug info from
hvmloader+ovmf+qemu+other-components-involved? With the broken ovmf, I got
nothing on the VM serial port, an empty, black vfb, nothing beyond "Invoking
OVMF..." in xl dmesg, and no errors in the qemu log file. Beyond bisecting, I
wasn't sure how to debug :-). I tried adding

device_model_args=[ "-debugcon file:debug.log", "-global 
isa-debugcon.iobase=0x402"]

to the VM config, but qemu failed to start with "-debugcon file:debug.log:
invalid option"

Regards,
Jim

[0] http://lists.xenproject.org/archives/html/xen-devel/2016-03/msg01626.html
[1] https://build.opensuse.org/package/show/Virtualization/ovmf


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] tools: Restrict configuration of qemu processes

[Jim Fehlig]

On 03/15/2016 06:28 AM, Wei Liu wrote:
> On Mon, Mar 14, 2016 at 07:14:12PM -0600, Jim Fehlig wrote:
>> Opps, forgot to cc the tools maintainers, sorry. I can resend if needed.
>>
>> Regards,
>> Jim
>>
>> On 03/14/2016 07:08 PM, Jim Fehlig wrote:
>>> Commit 6ef823fd added '-nodefaults' to the qemu args created by
>>> libxl, which is a good step in restricting qemu's default
>>> configuration. This change takes another step by adding
>>> -no-user-config, which ignores any user-provided config files in
>>> sysconfdir. Together, -nodefaults and -no-user-config allow Xen
>>> to avoid unkown and uncontrolled qemu configuration.
>>>
>>> Both options are also added to the qemu invocation in the
>>> xen-qemu-dom0-disk-backend systemd service file.
>>>
>>> Signed-off-by: Jim Fehlig <jfeh...@suse.com>
> Acked-by: Wei Liu <wei.l...@citrix.com>

If there are no remaining issues, can this patch be applied?  Thanks!

Regards,
Jim

>
>>> ---
>>>  tools/hotplug/Linux/systemd/xen-qemu-dom0-disk-backend.service.in | 1 +
>>>  tools/libxl/libxl_dm.c| 6 
>>> ++
>>>  2 files changed, 7 insertions(+)
>>>
>>> diff --git 
>>> a/tools/hotplug/Linux/systemd/xen-qemu-dom0-disk-backend.service.in 
>>> b/tools/hotplug/Linux/systemd/xen-qemu-dom0-disk-backend.service.in
>>> index acf61a8..f56775b 100644
>>> --- a/tools/hotplug/Linux/systemd/xen-qemu-dom0-disk-backend.service.in
>>> +++ b/tools/hotplug/Linux/systemd/xen-qemu-dom0-disk-backend.service.in
>>> @@ -14,6 +14,7 @@ ExecStartPre=/bin/mkdir -p @XEN_RUN_DIR@
>>>  ExecStart=@qemu_xen_systemd@ -xen-domid 0 \
>>> -xen-attach -name dom0 -nographic -M xenpv -daemonize \
>>> -monitor /dev/null -serial /dev/null -parallel /dev/null \
>>> +   -nodefaults -no-user-config \
>>> -pidfile @XEN_RUN_DIR@/qemu-dom0.pid
>>>  
>>>  [Install]
>>> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
>>> index 4aca38e..cfda24c 100644
>>> --- a/tools/libxl/libxl_dm.c
>>> +++ b/tools/libxl/libxl_dm.c
>>> @@ -828,6 +828,12 @@ static int 
>>> libxl__build_device_model_args_new(libxl__gc *gc,
>>>   */
>>>  flexarray_append(dm_args, "-nodefaults");
>>>  
>>> +/*
>>> + * Do not use any of the user-provided config files in sysconfdir,
>>> + * avoiding unkown and uncontrolled configuration.
>>> + */
>>> +flexarray_append(dm_args, "-no-user-config");
>>> +
>>>  if (b_info->type == LIBXL_DOMAIN_TYPE_PV) {
>>>  flexarray_append(dm_args, "-xen-attach");
>>>  }
> ___
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [libvirt test] 86625: regressions - FAIL

[Jim Fehlig]

On 03/19/2016 05:55 PM, osstest service owner wrote:
> flight 86625 libvirt real [real]
> http://logs.test-lab.xenproject.org/osstest/logs/86625/

There was a fair bit of turmoil after the libvirt NSS module got merged. AFAIK,
the various build issues and 'make check' failures have been resolved.

Regards,
Jim


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [libvirt] [PATCH V2 0/4] Extend to a tristate

[Jim Fehlig]

On 03/21/2016 06:49 AM, Ján Tomko wrote:
> On Mon, Feb 29, 2016 at 09:00:44PM -0700, Jim Fehlig wrote:
>> An expanded V2 of
>>
>> https://www.redhat.com/archives/libvir-list/2016-February/msg00140.html
>>
>> In V2, the  feature is extended with a state='on|off' attribute to
>> allow differentiating the 'on' and 'off' states with not set (or hypervisor
>> default).
>>
>> Obviously post 1.3.2 material. See individual patches for details.
>>
>> Jim Fehlig (4):
>>   conf: add 'state' attribute to  feature
>>   xenconfig: change 'hap' setting to align with Xen behavior
>>   Xen drivers: show hap enabled by default in capabilities
>>   libxl: support enabling and disabling  feature
>>
>>  docs/formatdomain.html.in  |  6 ++-
>>  docs/schemas/domaincommon.rng  |  6 ++-
>>  src/conf/domain_conf.c |  4 +-
>>  src/libxl/libxl_conf.c | 20 ++--
>>  src/xen/xen_hypervisor.c   |  2 +-
>>  src/xenconfig/xen_common.c | 14 ++---
>>  .../test-disk-positional-parms-full.cfg|  1 -
>>  .../test-disk-positional-parms-partial.cfg |  1 -
>>  ...est-fullvirt-direct-kernel-boot-bogus-extra.cfg |  1 -
>>  .../test-fullvirt-direct-kernel-boot-extra.cfg |  1 -
>>  .../test-fullvirt-direct-kernel-boot.cfg   |  1 -
>>  tests/xlconfigdata/test-fullvirt-multiusb.cfg  |  1 -
>>  tests/xlconfigdata/test-fullvirt-nohap.cfg | 26 ++
>>  tests/xlconfigdata/test-fullvirt-nohap.xml | 59 
>> ++
>>  tests/xlconfigdata/test-new-disk.cfg   |  1 -
>>  tests/xlconfigdata/test-rbd-multihost-noauth.cfg   |  1 -
>>  tests/xlconfigdata/test-spice-features.cfg |  1 -
>>  tests/xlconfigdata/test-spice.cfg  |  1 -
>>  tests/xlconfigdata/test-vif-rate.cfg   |  1 -
>>  tests/xlconfigtest.c   |  1 +
>>  tests/xmconfigdata/test-escape-paths.cfg   |  1 -
>>  .../xmconfigdata/test-fullvirt-default-feature.cfg |  1 -
>>  tests/xmconfigdata/test-fullvirt-force-hpet.cfg|  1 -
>>  tests/xmconfigdata/test-fullvirt-force-nohpet.cfg  |  1 -
>>  tests/xmconfigdata/test-fullvirt-localtime.cfg |  1 -
>>  tests/xmconfigdata/test-fullvirt-net-netfront.cfg  |  1 -
>>  tests/xmconfigdata/test-fullvirt-new-cdrom.cfg |  1 -
>>  tests/xmconfigdata/test-fullvirt-nohap.cfg | 28 ++
>>  tests/xmconfigdata/test-fullvirt-nohap.xml | 51 +++
>>  tests/xmconfigdata/test-fullvirt-parallel-tcp.cfg  |  1 -
>>  .../test-fullvirt-serial-dev-2-ports.cfg   |  1 -
>>  .../test-fullvirt-serial-dev-2nd-port.cfg  |  1 -
>>  tests/xmconfigdata/test-fullvirt-serial-file.cfg   |  1 -
>>  tests/xmconfigdata/test-fullvirt-serial-null.cfg   |  1 -
>>  tests/xmconfigdata/test-fullvirt-serial-pipe.cfg   |  1 -
>>  tests/xmconfigdata/test-fullvirt-serial-pty.cfg|  1 -
>>  tests/xmconfigdata/test-fullvirt-serial-stdio.cfg  |  1 -
>>  .../test-fullvirt-serial-tcp-telnet.cfg|  1 -
>>  tests/xmconfigdata/test-fullvirt-serial-tcp.cfg|  1 -
>>  tests/xmconfigdata/test-fullvirt-serial-udp.cfg|  1 -
>>  tests/xmconfigdata/test-fullvirt-serial-unix.cfg   |  1 -
>>  tests/xmconfigdata/test-fullvirt-sound.cfg |  1 -
>>  tests/xmconfigdata/test-fullvirt-usbmouse.cfg  |  1 -
>>  tests/xmconfigdata/test-fullvirt-usbtablet.cfg |  1 -
>>  tests/xmconfigdata/test-fullvirt-utc.cfg   |  1 -
>>  tests/xmconfigdata/test-no-source-cdrom.cfg|  1 -
>>  tests/xmconfigdata/test-pci-devs.cfg   |  1 -
>>  tests/xmconfigtest.c   |  1 +
>>  48 files changed, 202 insertions(+), 52 deletions(-)
>>  create mode 100644 tests/xlconfigdata/test-fullvirt-nohap.cfg
>>  create mode 100644 tests/xlconfigdata/test-fullvirt-nohap.xml
>>  create mode 100755 tests/xmconfigdata/test-fullvirt-nohap.cfg
>>  create mode 100644 tests/xmconfigdata/test-fullvirt-nohap.xml
>>
> ACK series

Thanks a lot for taking a look, much appreciated! Pushed now.

Regards,
Jim


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 7/8] docs: Document block-script protocol

[Jim Fehlig]

On 03/16/2016 10:09 AM, George Dunlap wrote:
> Signed-off-by: George Dunlap 
> ---
> CC: Ian Jackson 
> CC: Wei Liu 
> CC: Roger Pau Monne 
> ---
>  docs/misc/block-scripts.txt | 100 
> 
>  1 file changed, 100 insertions(+)
>
> diff --git a/docs/misc/block-scripts.txt b/docs/misc/block-scripts.txt
> new file mode 100644
> index 000..ef19207
> --- /dev/null
> +++ b/docs/misc/block-scripts.txt
> @@ -0,0 +1,100 @@
> +Block scripts
> +=
> +
> +Block scripts are called at the moment anytime blkback is directly
> +involved in providing access to a backend.  There are three general
> +cases this happens:
> +
> +1. When a user passes a block device in the 'target' field of the disk
> +specification
> +
> +2. When a user passes a file in the 'target' field of the disk
> +specification
> +
> +3. When a user specifies a custom script.
> +
> +Setup
> +-
> +
> +It is highly recommended that custom scripts as much as possible
> +include and use the common Xen functionality.  If the script is run
> +from the normal block script location (/etc/xen/scripts by default),
> +then this can be done by adding the following to the top of the
> +script:
> +
> +dir=$(dirname "$0")
> +. "$dir/block-common.sh"
> +
> +
> +Inputs
> +--
> +
> +In all cases, the scripts are called with either "add" or "remove" as
> +the command.  For custom scripts, the command will be the first
> +argument of the script (i.e. $1).
> +
> +The environment variable XENBUS_PATH will be set to the
> +path for the block device to be created.
> +
> +When the script is run, the following nodes shall already have been
> +written into xenstore:
> +
> + $XENBUS/paramsThe contents of the 'target' section of the disk 
> specification verbatim.
> + $XENBUS/mode  'r' (for readonly) or 'w' (for read-write)
> +
> +Output
> +---
> +
> +Block scripts are responsible for making sure that if a file is
> +provided to a VM read/write, that it is not provided to any other VM.
> +
> +FreeBSD block hotplug scripts must write
> +"$XENBUS_PATH/physical-device-path" with the path to the physical
> +device or file.  Linux and NetBSD block hotplug scripts *should* also
> +write this node.
> +
> +For the time being, Linux and NetBSD block hotplug scripts must write
> +"$XENBUS_PATH/physical-device" with the device's major and minor
> +numbers, written in hex, and separated by a colon.
> +
> +Scripts which include block-common.sh can simply call write_dev "$dev"
> +with a path to the device, and write_dev will do the right thing, now
> +and going forward.  (See the discussion below.)
> +
> +Rationale and future work
> +-
> +
> +Historically, the block scripts wrote a node called "physical-device",
> +which contains the major and minor numbers, written in hex, and
> +separated by a colon (e.g., "1a:2").  This is required by the Linux
> +blkback driver.
> +
> +FreeBSD blkback, on the other hand, does not have the concept of
> +major/minor numbers, and can give direct access to a file without
> +going through loopback; so its driver will consume
> +physical-device-path.
> +
> +On Linux, the device model (qemu) needs access to a file it can
> +interpret to provide emulated disks before paravirtualized drivers are
> +marked as up.  The easiest way to accomplish this is to allow qemu to
> +consume physical-device-path (rather than, say, having dom0 act as
> +both a frontend and a backend).
> +
> +Going forward, the plan is at some point to have all block scripts
> +simply write "physical-device-path", and then have libxl write the
> +other nodes.  The reason we haven't done this yet is that the main
> +block script wants to check to make sure the *major/minor* number
> +hasn't been re-used, rather than just checking that the *specific
> +device node* isn't re-used.  To do this it currently uses
> +physical-device; and to do this *safely* it needs physical-device to
> +be written with the lock held.
> +
> +The simplest solution for sorting this out would be to have the block
> +script use physical-device if it's present, but if not, to directly
> +stat physical-device-path.  But there's not time before the 4.7
> +release to make sure all that works.
> +
> +Another possibility would be to do away with the block scripts
> +altogether when not actually running any scripts,

Just to clarify, do you mean drop support for all block scripts?

>  and do the duplicate
> +checking inside of libxl.  The rationale for doing this in block
> +scripts rather than in libxl isn't clear at thes point.

Block scripts like block-iscsi and block-drbd also "cook" $XENBUS/params into
$XENBUS_PATH/physical-device{,-path} right?

Regards,
Jim

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v4 04/14] firmware/makefile: install BIOS blob ...

[Jim Fehlig]

On 03/17/2016 12:33 PM, Anthony PERARD wrote:
> On Thu, Mar 17, 2016 at 12:37:36PM -0500, Doug Goldstein wrote:
>> On 3/14/16 12:55 PM, Anthony PERARD wrote:
>>> ... into the firmware directory, along with hvmloader.
>>>
>>> Signed-off-by: Anthony PERARD 
>>> ---
>>> Change in V4:
>>> - remove install of acpi dsdt table
>>>
>>> Change in V3:
>>> - do not check if ROMs file exist before installing, they should exist
>>> - change rules for dsdt_anycpu_qemu_xen.c in oder to generate both .c and
>>>   .aml files without changing temporarly the other dsdt_*.c rules.
>>> ---
>>>  tools/firmware/Makefile | 13 +
>>>  1 file changed, 13 insertions(+)
>>>
>>> diff --git a/tools/firmware/Makefile b/tools/firmware/Makefile
>>> index 6cc86ce..6a37758 100644
>>> --- a/tools/firmware/Makefile
>>> +++ b/tools/firmware/Makefile
>>> @@ -19,6 +19,9 @@ SUBDIRS-y += hvmloader
>>>  
>>>  LD32BIT-$(CONFIG_FreeBSD) := LD32BIT_FLAG=-melf_i386_fbsd
>>>  
>>> +SEABIOS_ROM := seabios-dir/out/bios.bin
>>> +OVMF_ROM := ovmf-dir/ovmf.bin
>>> +
>>>  ovmf-dir:
>>> GIT=$(GIT) $(XEN_ROOT)/scripts/git-checkout.sh $(OVMF_UPSTREAM_URL) 
>>> $(OVMF_UPSTREAM_REVISION) ovmf-dir
>>> cp ovmf-makefile ovmf-dir/Makefile;
>>> @@ -45,6 +48,16 @@ endif
>>>  install: all
>>> [ -d $(INST_DIR) ] || $(INSTALL_DIR) $(INST_DIR)
>>> [ ! -e $(TARGET) ] || $(INSTALL_DATA) $(TARGET) $(INST_DIR)
>>> +ifeq ($(CONFIG_SEABIOS),y)
>>> +ifeq ($(SEABIOS_PATH),)
>>> +   $(INSTALL_DATA) $(SEABIOS_ROM) $(INST_DIR)/seabios.bin
>> Why install this as "seabios.bin" when the default is "bios.bin". Most
>> distro's packages for SeaBIOS install it as "bios.bin"
> No reason. I guess it's fine to keep the same name ("bios.bin"). My distro
> install it as "bios-256k.bin", with "bios.bin" been the small version I
> guess.

Same with SUSE's SeaBIOS package, which contains bios.bin (sz 131072) and
bios-256k.bin (sz 262144).

Regards,
Jim


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [libvirt] [PATCH V2 0/4] Extend to a tristate

[Jim Fehlig]

Ping V2 :-).

FYI, Joao has {Reviewed,Tested}-by the series. Thanks!

Regards,
Jim

On 03/07/2016 09:02 PM, Jim Fehlig wrote:
> On 02/29/2016 09:00 PM, Jim Fehlig wrote:
>> An expanded V2 of
>>
>> https://www.redhat.com/archives/libvir-list/2016-February/msg00140.html
>>
>> In V2, the  feature is extended with a state='on|off' attribute to
>> allow differentiating the 'on' and 'off' states with not set (or hypervisor
>> default).
> Any comments about this approach? I essentially followed the same pattern as 
> the
>  and  hypervisor features.  Thanks!
>
> Regards,
> Jim
>
>> Obviously post 1.3.2 material. See individual patches for details.
>>
>> Jim Fehlig (4):
>>   conf: add 'state' attribute to  feature
>>   xenconfig: change 'hap' setting to align with Xen behavior
>>   Xen drivers: show hap enabled by default in capabilities
>>   libxl: support enabling and disabling  feature
>>
>>  docs/formatdomain.html.in  |  6 ++-
>>  docs/schemas/domaincommon.rng  |  6 ++-
>>  src/conf/domain_conf.c |  4 +-
>>  src/libxl/libxl_conf.c | 20 ++--
>>  src/xen/xen_hypervisor.c   |  2 +-
>>  src/xenconfig/xen_common.c | 14 ++---
>>  .../test-disk-positional-parms-full.cfg|  1 -
>>  .../test-disk-positional-parms-partial.cfg |  1 -
>>  ...est-fullvirt-direct-kernel-boot-bogus-extra.cfg |  1 -
>>  .../test-fullvirt-direct-kernel-boot-extra.cfg |  1 -
>>  .../test-fullvirt-direct-kernel-boot.cfg   |  1 -
>>  tests/xlconfigdata/test-fullvirt-multiusb.cfg  |  1 -
>>  tests/xlconfigdata/test-fullvirt-nohap.cfg | 26 ++
>>  tests/xlconfigdata/test-fullvirt-nohap.xml | 59 
>> ++
>>  tests/xlconfigdata/test-new-disk.cfg   |  1 -
>>  tests/xlconfigdata/test-rbd-multihost-noauth.cfg   |  1 -
>>  tests/xlconfigdata/test-spice-features.cfg |  1 -
>>  tests/xlconfigdata/test-spice.cfg  |  1 -
>>  tests/xlconfigdata/test-vif-rate.cfg   |  1 -
>>  tests/xlconfigtest.c   |  1 +
>>  tests/xmconfigdata/test-escape-paths.cfg   |  1 -
>>  .../xmconfigdata/test-fullvirt-default-feature.cfg |  1 -
>>  tests/xmconfigdata/test-fullvirt-force-hpet.cfg|  1 -
>>  tests/xmconfigdata/test-fullvirt-force-nohpet.cfg  |  1 -
>>  tests/xmconfigdata/test-fullvirt-localtime.cfg |  1 -
>>  tests/xmconfigdata/test-fullvirt-net-netfront.cfg  |  1 -
>>  tests/xmconfigdata/test-fullvirt-new-cdrom.cfg |  1 -
>>  tests/xmconfigdata/test-fullvirt-nohap.cfg | 28 ++
>>  tests/xmconfigdata/test-fullvirt-nohap.xml | 51 +++
>>  tests/xmconfigdata/test-fullvirt-parallel-tcp.cfg  |  1 -
>>  .../test-fullvirt-serial-dev-2-ports.cfg   |  1 -
>>  .../test-fullvirt-serial-dev-2nd-port.cfg  |  1 -
>>  tests/xmconfigdata/test-fullvirt-serial-file.cfg   |  1 -
>>  tests/xmconfigdata/test-fullvirt-serial-null.cfg   |  1 -
>>  tests/xmconfigdata/test-fullvirt-serial-pipe.cfg   |  1 -
>>  tests/xmconfigdata/test-fullvirt-serial-pty.cfg|  1 -
>>  tests/xmconfigdata/test-fullvirt-serial-stdio.cfg  |  1 -
>>  .../test-fullvirt-serial-tcp-telnet.cfg|  1 -
>>  tests/xmconfigdata/test-fullvirt-serial-tcp.cfg|  1 -
>>  tests/xmconfigdata/test-fullvirt-serial-udp.cfg|  1 -
>>  tests/xmconfigdata/test-fullvirt-serial-unix.cfg   |  1 -
>>  tests/xmconfigdata/test-fullvirt-sound.cfg |  1 -
>>  tests/xmconfigdata/test-fullvirt-usbmouse.cfg  |  1 -
>>  tests/xmconfigdata/test-fullvirt-usbtablet.cfg |  1 -
>>  tests/xmconfigdata/test-fullvirt-utc.cfg   |  1 -
>>  tests/xmconfigdata/test-no-source-cdrom.cfg|  1 -
>>  tests/xmconfigdata/test-pci-devs.cfg   |  1 -
>>  tests/xmconfigtest.c   |  1 +
>>  48 files changed, 202 insertions(+), 52 deletions(-)
>>  create mode 100644 tests/xlconfigdata/test-fullvirt-nohap.cfg
>>  create mode 100644 tests/xlconfigdata/test-fullvirt-nohap.xml
>>  create mode 100755 tests/xmconfigdata/test-fullvirt-nohap.cfg
>>  create mode 100644 tests/xmconfigdata/test-fullvirt-nohap.xml
>>
> --
> libvir-list mailing list
> libvir-l...@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] tools: Restrict configuration of qemu processes

[Jim Fehlig]

Opps, forgot to cc the tools maintainers, sorry. I can resend if needed.

Regards,
Jim

On 03/14/2016 07:08 PM, Jim Fehlig wrote:
> Commit 6ef823fd added '-nodefaults' to the qemu args created by
> libxl, which is a good step in restricting qemu's default
> configuration. This change takes another step by adding
> -no-user-config, which ignores any user-provided config files in
> sysconfdir. Together, -nodefaults and -no-user-config allow Xen
> to avoid unkown and uncontrolled qemu configuration.
>
> Both options are also added to the qemu invocation in the
> xen-qemu-dom0-disk-backend systemd service file.
>
> Signed-off-by: Jim Fehlig <jfeh...@suse.com>
> ---
>  tools/hotplug/Linux/systemd/xen-qemu-dom0-disk-backend.service.in | 1 +
>  tools/libxl/libxl_dm.c| 6 ++
>  2 files changed, 7 insertions(+)
>
> diff --git 
> a/tools/hotplug/Linux/systemd/xen-qemu-dom0-disk-backend.service.in 
> b/tools/hotplug/Linux/systemd/xen-qemu-dom0-disk-backend.service.in
> index acf61a8..f56775b 100644
> --- a/tools/hotplug/Linux/systemd/xen-qemu-dom0-disk-backend.service.in
> +++ b/tools/hotplug/Linux/systemd/xen-qemu-dom0-disk-backend.service.in
> @@ -14,6 +14,7 @@ ExecStartPre=/bin/mkdir -p @XEN_RUN_DIR@
>  ExecStart=@qemu_xen_systemd@ -xen-domid 0 \
>   -xen-attach -name dom0 -nographic -M xenpv -daemonize \
>   -monitor /dev/null -serial /dev/null -parallel /dev/null \
> + -nodefaults -no-user-config \
>   -pidfile @XEN_RUN_DIR@/qemu-dom0.pid
>  
>  [Install]
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index 4aca38e..cfda24c 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -828,6 +828,12 @@ static int libxl__build_device_model_args_new(libxl__gc 
> *gc,
>   */
>  flexarray_append(dm_args, "-nodefaults");
>  
> +/*
> + * Do not use any of the user-provided config files in sysconfdir,
> + * avoiding unkown and uncontrolled configuration.
> + */
> +flexarray_append(dm_args, "-no-user-config");
> +
>  if (b_info->type == LIBXL_DOMAIN_TYPE_PV) {
>  flexarray_append(dm_args, "-xen-attach");
>  }


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH] tools: Restrict configuration of qemu processes

[Jim Fehlig]

Commit 6ef823fd added '-nodefaults' to the qemu args created by
libxl, which is a good step in restricting qemu's default
configuration. This change takes another step by adding
-no-user-config, which ignores any user-provided config files in
sysconfdir. Together, -nodefaults and -no-user-config allow Xen
to avoid unkown and uncontrolled qemu configuration.

Both options are also added to the qemu invocation in the
xen-qemu-dom0-disk-backend systemd service file.

Signed-off-by: Jim Fehlig <jfeh...@suse.com>
---
 tools/hotplug/Linux/systemd/xen-qemu-dom0-disk-backend.service.in | 1 +
 tools/libxl/libxl_dm.c| 6 ++
 2 files changed, 7 insertions(+)

diff --git a/tools/hotplug/Linux/systemd/xen-qemu-dom0-disk-backend.service.in 
b/tools/hotplug/Linux/systemd/xen-qemu-dom0-disk-backend.service.in
index acf61a8..f56775b 100644
--- a/tools/hotplug/Linux/systemd/xen-qemu-dom0-disk-backend.service.in
+++ b/tools/hotplug/Linux/systemd/xen-qemu-dom0-disk-backend.service.in
@@ -14,6 +14,7 @@ ExecStartPre=/bin/mkdir -p @XEN_RUN_DIR@
 ExecStart=@qemu_xen_systemd@ -xen-domid 0 \
-xen-attach -name dom0 -nographic -M xenpv -daemonize \
-monitor /dev/null -serial /dev/null -parallel /dev/null \
+   -nodefaults -no-user-config \
-pidfile @XEN_RUN_DIR@/qemu-dom0.pid
 
 [Install]
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 4aca38e..cfda24c 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -828,6 +828,12 @@ static int libxl__build_device_model_args_new(libxl__gc 
*gc,
  */
 flexarray_append(dm_args, "-nodefaults");
 
+/*
+ * Do not use any of the user-provided config files in sysconfdir,
+ * avoiding unkown and uncontrolled configuration.
+ */
+flexarray_append(dm_args, "-no-user-config");
+
 if (b_info->type == LIBXL_DOMAIN_TYPE_PV) {
 flexarray_append(dm_args, "-xen-attach");
 }
-- 
2.1.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [RFC] tools: don't use qemu default config

[Jim Fehlig]

On 03/11/2016 03:28 PM, Konrad Rzeszutek Wilk wrote:
> On Fri, Mar 11, 2016 at 03:08:30PM -0700, Jim Fehlig wrote:
>> I recently changed SUSE's Xen package to use the distro qemu instead of 
>> building
>> qemu-xen. This got some other eyes looking at Xen's use of qemu and it was
>> noticed that libxl and xen-qemu-dom0-disk-backend.service do not include
>> '-no-user-config' when invoking qemu. The latter also does not include
>> '-nodefaults'. Commit 6ef823fd added '-nodefaults' to the qemu args created 
>> by
>> libxl, but missed adding it to the qemu args in 
>> xen-qemu-dom0-disk-backend.service.
>>
>> I _think_ adding '-nodefaults' to the qemu args in the service file is
>> non-controversial. What do folks think of also adding '-no-user-config'? It
>> seems the global config in /etc/qemu/qemu.conf would end up being more
>> problematic than helpful for Xen.
> Is there a description (or URL) of what one can jam in there?

I failed to find one. I asked a few SUSE qemu devs and they too had no
documentation hints, but noted "pretty much anything that fits in the command
line can go into the config file these days".

The user config files are .ini-style. Command line option  '-opt_a foo=1,bar=2'
becomes

  [opt_a]
foo=1
bar=2

Regards,
Jim


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [RFC] tools: don't use qemu default config

[Jim Fehlig]

I recently changed SUSE's Xen package to use the distro qemu instead of building
qemu-xen. This got some other eyes looking at Xen's use of qemu and it was
noticed that libxl and xen-qemu-dom0-disk-backend.service do not include
'-no-user-config' when invoking qemu. The latter also does not include
'-nodefaults'. Commit 6ef823fd added '-nodefaults' to the qemu args created by
libxl, but missed adding it to the qemu args in 
xen-qemu-dom0-disk-backend.service.

I _think_ adding '-nodefaults' to the qemu args in the service file is
non-controversial. What do folks think of also adding '-no-user-config'? It
seems the global config in /etc/qemu/qemu.conf would end up being more
problematic than helpful for Xen.

As a side note, the libvirt qemu driver includes '-no-user-config -nodefaults'
in all its qemu invocations to avoid configuration which it doesn't control.

WRT qemu args, another suggestion was to explicitly specify 'accel=xen' in the
machine arg. Together, these changes would e.g. result in the service file qemu
args changing slightly to

  -machine xenpv,accel=xen -xen-domid 0 -xen-attach -name dom0 \
  -daemonize -no-user-config -nodefaults -display none \
  -pidfile /var/run/xen/qemu-dom0.pid

If folks agree with these changes, I'll be happy to provide a patches for libxl
and the systemd service file. Thanks for your comments.

Regards,
Jim

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [libvirt] [PATCH V2 1/4] conf: add 'state' attribute to feature

[Jim Fehlig]

Joao Martins wrote:
> 
> On 03/01/2016 04:00 AM, Jim Fehlig wrote:
>> Most hypervisors use Hardware Assisted Paging by default and don't
>> require specifying the feature in domain conf. But some hypervisors
>> support disabling HAP on a per-domain basis. To enable HAP by default
>> yet provide a knob to disable it, extend the  feature with a
>> 'state=on|off' attribute, similar to  and  features.
>>
>> In the absence of , the hypervisor default (on) is used. 
>> without the state attribute would be the same as  for
>> backwards compatibility. And of course  disables hap.
>>
>> Signed-off-by: Jim Fehlig <jfeh...@suse.com>
>> ---
>>  docs/formatdomain.html.in | 6 --
>>  docs/schemas/domaincommon.rng | 6 +-
>>  src/conf/domain_conf.c| 4 ++--
>>  3 files changed, 11 insertions(+), 5 deletions(-)
>>
>> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
>> index 5016772..c06bcf3 100644
>> --- a/docs/formatdomain.html.in
>> +++ b/docs/formatdomain.html.in
>> @@ -1494,8 +1494,10 @@
>>Interrupt) for the guest.
>>
>>hap
>> -  Enable use of Hardware Assisted Paging if available in
>> -the hardware.
>> +  Depending on the state attribute (values 
>> on,
>> +off) enable or disable use of Hardware Assisted Paging.
>> +The default is on if the hypervisor detects 
>> availability
>> +of Hardware Assisted Paging.
>>
>>viridian
>>Enable Viridian hypervisor extensions for paravirtualizing
>> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
>> index 67af93a..dd6e93a 100644
>> --- a/docs/schemas/domaincommon.rng
>> +++ b/docs/schemas/domaincommon.rng
>> @@ -4108,7 +4108,11 @@
>>
>>
>>  
>> -  
>> +  
>> +
>> +  
>> +
>> +  
> Perhaps  would be better (see chunk below) ? That 
> one
> appears to be a reference of what you are adding above, and it's the same as
> pvspinlock. Though some other elements don't appear to use this, not sure why.

Ah, thanks for catching that. 'featurestate' is definitely the better choice 
here.

> 
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 89d3a6b..141122c 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -4132,9 +4132,7 @@
>
>  
>
> -
> -  
> -
> +
>
>  
>
> 
> Other that,
> 
> Reviewed-by: Joao Martins <joao.m.mart...@oracle.com>

Thanks. I've squashed in your diff, but should probably wait for any additional
comments before pushing this series.

Regards,
Jim

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [libvirt] [PATCH V2 0/4] Extend to a tristate

[Jim Fehlig]

On 02/29/2016 09:00 PM, Jim Fehlig wrote:
> An expanded V2 of
>
> https://www.redhat.com/archives/libvir-list/2016-February/msg00140.html
>
> In V2, the  feature is extended with a state='on|off' attribute to
> allow differentiating the 'on' and 'off' states with not set (or hypervisor
> default).

Any comments about this approach? I essentially followed the same pattern as the
 and  hypervisor features.  Thanks!

Regards,
Jim

>
> Obviously post 1.3.2 material. See individual patches for details.
>
> Jim Fehlig (4):
>   conf: add 'state' attribute to  feature
>   xenconfig: change 'hap' setting to align with Xen behavior
>   Xen drivers: show hap enabled by default in capabilities
>   libxl: support enabling and disabling  feature
>
>  docs/formatdomain.html.in  |  6 ++-
>  docs/schemas/domaincommon.rng  |  6 ++-
>  src/conf/domain_conf.c |  4 +-
>  src/libxl/libxl_conf.c | 20 ++--
>  src/xen/xen_hypervisor.c   |  2 +-
>  src/xenconfig/xen_common.c | 14 ++---
>  .../test-disk-positional-parms-full.cfg|  1 -
>  .../test-disk-positional-parms-partial.cfg |  1 -
>  ...est-fullvirt-direct-kernel-boot-bogus-extra.cfg |  1 -
>  .../test-fullvirt-direct-kernel-boot-extra.cfg |  1 -
>  .../test-fullvirt-direct-kernel-boot.cfg   |  1 -
>  tests/xlconfigdata/test-fullvirt-multiusb.cfg  |  1 -
>  tests/xlconfigdata/test-fullvirt-nohap.cfg | 26 ++
>  tests/xlconfigdata/test-fullvirt-nohap.xml | 59 
> ++
>  tests/xlconfigdata/test-new-disk.cfg   |  1 -
>  tests/xlconfigdata/test-rbd-multihost-noauth.cfg   |  1 -
>  tests/xlconfigdata/test-spice-features.cfg |  1 -
>  tests/xlconfigdata/test-spice.cfg  |  1 -
>  tests/xlconfigdata/test-vif-rate.cfg   |  1 -
>  tests/xlconfigtest.c   |  1 +
>  tests/xmconfigdata/test-escape-paths.cfg   |  1 -
>  .../xmconfigdata/test-fullvirt-default-feature.cfg |  1 -
>  tests/xmconfigdata/test-fullvirt-force-hpet.cfg|  1 -
>  tests/xmconfigdata/test-fullvirt-force-nohpet.cfg  |  1 -
>  tests/xmconfigdata/test-fullvirt-localtime.cfg |  1 -
>  tests/xmconfigdata/test-fullvirt-net-netfront.cfg  |  1 -
>  tests/xmconfigdata/test-fullvirt-new-cdrom.cfg |  1 -
>  tests/xmconfigdata/test-fullvirt-nohap.cfg | 28 ++
>  tests/xmconfigdata/test-fullvirt-nohap.xml | 51 +++
>  tests/xmconfigdata/test-fullvirt-parallel-tcp.cfg  |  1 -
>  .../test-fullvirt-serial-dev-2-ports.cfg   |  1 -
>  .../test-fullvirt-serial-dev-2nd-port.cfg  |  1 -
>  tests/xmconfigdata/test-fullvirt-serial-file.cfg   |  1 -
>  tests/xmconfigdata/test-fullvirt-serial-null.cfg   |  1 -
>  tests/xmconfigdata/test-fullvirt-serial-pipe.cfg   |  1 -
>  tests/xmconfigdata/test-fullvirt-serial-pty.cfg|  1 -
>  tests/xmconfigdata/test-fullvirt-serial-stdio.cfg  |  1 -
>  .../test-fullvirt-serial-tcp-telnet.cfg|  1 -
>  tests/xmconfigdata/test-fullvirt-serial-tcp.cfg|  1 -
>  tests/xmconfigdata/test-fullvirt-serial-udp.cfg|  1 -
>  tests/xmconfigdata/test-fullvirt-serial-unix.cfg   |  1 -
>  tests/xmconfigdata/test-fullvirt-sound.cfg |  1 -
>  tests/xmconfigdata/test-fullvirt-usbmouse.cfg  |  1 -
>  tests/xmconfigdata/test-fullvirt-usbtablet.cfg |  1 -
>  tests/xmconfigdata/test-fullvirt-utc.cfg   |  1 -
>  tests/xmconfigdata/test-no-source-cdrom.cfg|  1 -
>  tests/xmconfigdata/test-pci-devs.cfg   |  1 -
>  tests/xmconfigtest.c   |  1 +
>  48 files changed, 202 insertions(+), 52 deletions(-)
>  create mode 100644 tests/xlconfigdata/test-fullvirt-nohap.cfg
>  create mode 100644 tests/xlconfigdata/test-fullvirt-nohap.xml
>  create mode 100755 tests/xmconfigdata/test-fullvirt-nohap.cfg
>  create mode 100644 tests/xmconfigdata/test-fullvirt-nohap.xml
>


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH V2 0/4] Extend to a tristate

[Jim Fehlig]

An expanded V2 of

https://www.redhat.com/archives/libvir-list/2016-February/msg00140.html

In V2, the  feature is extended with a state='on|off' attribute to
allow differentiating the 'on' and 'off' states with not set (or hypervisor
default).

Obviously post 1.3.2 material. See individual patches for details.

Jim Fehlig (4):
  conf: add 'state' attribute to  feature
  xenconfig: change 'hap' setting to align with Xen behavior
  Xen drivers: show hap enabled by default in capabilities
  libxl: support enabling and disabling  feature

 docs/formatdomain.html.in  |  6 ++-
 docs/schemas/domaincommon.rng  |  6 ++-
 src/conf/domain_conf.c |  4 +-
 src/libxl/libxl_conf.c | 20 ++--
 src/xen/xen_hypervisor.c   |  2 +-
 src/xenconfig/xen_common.c | 14 ++---
 .../test-disk-positional-parms-full.cfg|  1 -
 .../test-disk-positional-parms-partial.cfg |  1 -
 ...est-fullvirt-direct-kernel-boot-bogus-extra.cfg |  1 -
 .../test-fullvirt-direct-kernel-boot-extra.cfg |  1 -
 .../test-fullvirt-direct-kernel-boot.cfg   |  1 -
 tests/xlconfigdata/test-fullvirt-multiusb.cfg  |  1 -
 tests/xlconfigdata/test-fullvirt-nohap.cfg | 26 ++
 tests/xlconfigdata/test-fullvirt-nohap.xml | 59 ++
 tests/xlconfigdata/test-new-disk.cfg   |  1 -
 tests/xlconfigdata/test-rbd-multihost-noauth.cfg   |  1 -
 tests/xlconfigdata/test-spice-features.cfg |  1 -
 tests/xlconfigdata/test-spice.cfg  |  1 -
 tests/xlconfigdata/test-vif-rate.cfg   |  1 -
 tests/xlconfigtest.c   |  1 +
 tests/xmconfigdata/test-escape-paths.cfg   |  1 -
 .../xmconfigdata/test-fullvirt-default-feature.cfg |  1 -
 tests/xmconfigdata/test-fullvirt-force-hpet.cfg|  1 -
 tests/xmconfigdata/test-fullvirt-force-nohpet.cfg  |  1 -
 tests/xmconfigdata/test-fullvirt-localtime.cfg |  1 -
 tests/xmconfigdata/test-fullvirt-net-netfront.cfg  |  1 -
 tests/xmconfigdata/test-fullvirt-new-cdrom.cfg |  1 -
 tests/xmconfigdata/test-fullvirt-nohap.cfg | 28 ++
 tests/xmconfigdata/test-fullvirt-nohap.xml | 51 +++
 tests/xmconfigdata/test-fullvirt-parallel-tcp.cfg  |  1 -
 .../test-fullvirt-serial-dev-2-ports.cfg   |  1 -
 .../test-fullvirt-serial-dev-2nd-port.cfg  |  1 -
 tests/xmconfigdata/test-fullvirt-serial-file.cfg   |  1 -
 tests/xmconfigdata/test-fullvirt-serial-null.cfg   |  1 -
 tests/xmconfigdata/test-fullvirt-serial-pipe.cfg   |  1 -
 tests/xmconfigdata/test-fullvirt-serial-pty.cfg|  1 -
 tests/xmconfigdata/test-fullvirt-serial-stdio.cfg  |  1 -
 .../test-fullvirt-serial-tcp-telnet.cfg|  1 -
 tests/xmconfigdata/test-fullvirt-serial-tcp.cfg|  1 -
 tests/xmconfigdata/test-fullvirt-serial-udp.cfg|  1 -
 tests/xmconfigdata/test-fullvirt-serial-unix.cfg   |  1 -
 tests/xmconfigdata/test-fullvirt-sound.cfg |  1 -
 tests/xmconfigdata/test-fullvirt-usbmouse.cfg  |  1 -
 tests/xmconfigdata/test-fullvirt-usbtablet.cfg |  1 -
 tests/xmconfigdata/test-fullvirt-utc.cfg   |  1 -
 tests/xmconfigdata/test-no-source-cdrom.cfg|  1 -
 tests/xmconfigdata/test-pci-devs.cfg   |  1 -
 tests/xmconfigtest.c   |  1 +
 48 files changed, 202 insertions(+), 52 deletions(-)
 create mode 100644 tests/xlconfigdata/test-fullvirt-nohap.cfg
 create mode 100644 tests/xlconfigdata/test-fullvirt-nohap.xml
 create mode 100755 tests/xmconfigdata/test-fullvirt-nohap.cfg
 create mode 100644 tests/xmconfigdata/test-fullvirt-nohap.xml

-- 
2.1.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH V2 4/4] libxl: support enabling and disabling feature

[Jim Fehlig]

Until now, the libxl driver ignored any  setting in domain XML
and deferred to libxl, which enables hap if not specified. While
this is a good default, it prevents disabling hap if desired.

This change allows disabling hap with . hap is
explicitly enabled with  or 

[Xen-devel] [PATCH V2 2/4] xenconfig: change 'hap' setting to align with Xen behavior

[Jim Fehlig]

hap is enabled by default in xm and xl config and usually only
specified when it is desirable to disable hap (hap = 0). Change
the xm,xl <-> xml converter to behave similarly. I.e. only
produce 'hap = 0' when  and vice versa.

Signed-off-by: Jim Fehlig <jfeh...@suse.com>
---
 src/xenconfig/xen_common.c | 14 ++---
 .../test-disk-positional-parms-full.cfg|  1 -
 .../test-disk-positional-parms-partial.cfg |  1 -
 ...est-fullvirt-direct-kernel-boot-bogus-extra.cfg |  1 -
 .../test-fullvirt-direct-kernel-boot-extra.cfg |  1 -
 .../test-fullvirt-direct-kernel-boot.cfg   |  1 -
 tests/xlconfigdata/test-fullvirt-multiusb.cfg  |  1 -
 tests/xlconfigdata/test-fullvirt-nohap.cfg | 26 ++
 tests/xlconfigdata/test-fullvirt-nohap.xml | 59 ++
 tests/xlconfigdata/test-new-disk.cfg   |  1 -
 tests/xlconfigdata/test-rbd-multihost-noauth.cfg   |  1 -
 tests/xlconfigdata/test-spice-features.cfg |  1 -
 tests/xlconfigdata/test-spice.cfg  |  1 -
 tests/xlconfigdata/test-vif-rate.cfg   |  1 -
 tests/xlconfigtest.c   |  1 +
 tests/xmconfigdata/test-escape-paths.cfg   |  1 -
 .../xmconfigdata/test-fullvirt-default-feature.cfg |  1 -
 tests/xmconfigdata/test-fullvirt-force-hpet.cfg|  1 -
 tests/xmconfigdata/test-fullvirt-force-nohpet.cfg  |  1 -
 tests/xmconfigdata/test-fullvirt-localtime.cfg |  1 -
 tests/xmconfigdata/test-fullvirt-net-netfront.cfg  |  1 -
 tests/xmconfigdata/test-fullvirt-new-cdrom.cfg |  1 -
 tests/xmconfigdata/test-fullvirt-nohap.cfg | 28 ++
 tests/xmconfigdata/test-fullvirt-nohap.xml | 51 +++
 tests/xmconfigdata/test-fullvirt-parallel-tcp.cfg  |  1 -
 .../test-fullvirt-serial-dev-2-ports.cfg   |  1 -
 .../test-fullvirt-serial-dev-2nd-port.cfg  |  1 -
 tests/xmconfigdata/test-fullvirt-serial-file.cfg   |  1 -
 tests/xmconfigdata/test-fullvirt-serial-null.cfg   |  1 -
 tests/xmconfigdata/test-fullvirt-serial-pipe.cfg   |  1 -
 tests/xmconfigdata/test-fullvirt-serial-pty.cfg|  1 -
 tests/xmconfigdata/test-fullvirt-serial-stdio.cfg  |  1 -
 .../test-fullvirt-serial-tcp-telnet.cfg|  1 -
 tests/xmconfigdata/test-fullvirt-serial-tcp.cfg|  1 -
 tests/xmconfigdata/test-fullvirt-serial-udp.cfg|  1 -
 tests/xmconfigdata/test-fullvirt-serial-unix.cfg   |  1 -
 tests/xmconfigdata/test-fullvirt-sound.cfg |  1 -
 tests/xmconfigdata/test-fullvirt-usbmouse.cfg  |  1 -
 tests/xmconfigdata/test-fullvirt-usbtablet.cfg |  1 -
 tests/xmconfigdata/test-fullvirt-utc.cfg   |  1 -
 tests/xmconfigdata/test-no-source-cdrom.cfg|  1 -
 tests/xmconfigdata/test-pci-devs.cfg   |  1 -
 tests/xmconfigtest.c   |  1 +
 43 files changed, 173 insertions(+), 43 deletions(-)

diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c
index 828c8e9..4dcd484 100644
--- a/src/xenconfig/xen_common.c
+++ b/src/xenconfig/xen_common.c
@@ -528,11 +528,11 @@ xenParseCPUFeatures(virConfPtr conf, virDomainDefPtr def)
 
 else if (val)
 def->features[VIR_DOMAIN_FEATURE_APIC] = VIR_TRISTATE_SWITCH_ON;
-if (xenConfigGetBool(conf, "hap", , 0) < 0)
+if (xenConfigGetBool(conf, "hap", , 1) < 0)
 return -1;
 
-else if (val)
-def->features[VIR_DOMAIN_FEATURE_HAP] = VIR_TRISTATE_SWITCH_ON;
+else if (!val)
+def->features[VIR_DOMAIN_FEATURE_HAP] = VIR_TRISTATE_SWITCH_OFF;
 if (xenConfigGetBool(conf, "viridian", , 0) < 0)
 return -1;
 
@@ -1572,10 +1572,10 @@ xenFormatCPUFeatures(virConfPtr conf, virDomainDefPtr 
def)
 VIR_TRISTATE_SWITCH_ON) ? 1 : 0) < 0)
 return -1;
 
-if (xenConfigSetInt(conf, "hap",
-(def->features[VIR_DOMAIN_FEATURE_HAP] ==
- VIR_TRISTATE_SWITCH_ON) ? 1 : 0) < 0)
-return -1;
+if (def->features[VIR_DOMAIN_FEATURE_HAP] == VIR_TRISTATE_SWITCH_OFF) {
+if (xenConfigSetInt(conf, "hap", 0) < 0)
+return -1;
+}
 
 if (xenConfigSetInt(conf, "viridian",
 (def->features[VIR_DOMAIN_FEATURE_VIRIDIAN] ==
diff --git a/tests/xlconfigdata/test-disk-positional-parms-full.cfg 
b/tests/xlconfigdata/test-disk-positional-parms-full.cfg
index 026e451..c5bbb03 100644
--- a/tests/xlconfigdata/test-disk-positional-parms-full.cfg
+++ b/tests/xlconfigdata/test-disk-positional-parms-full.cfg
@@ -6,7 +6,6 @@ vcpus = 1
 pae = 1
 acpi = 1
 apic = 1
-hap = 0
 viridian = 0
 rtc_timeoffset = 0
 localtime = 0
diff --git a/tests/xlconfigdata/test-disk-positional-parms-partial.cfg 
b/tests/xlconfigdata/test-disk-positional-parms-par

[Xen-devel] [PATCH V2 1/4] conf: add 'state' attribute to feature

[Jim Fehlig]

Most hypervisors use Hardware Assisted Paging by default and don't
require specifying the feature in domain conf. But some hypervisors
support disabling HAP on a per-domain basis. To enable HAP by default
yet provide a knob to disable it, extend the  feature with a
'state=on|off' attribute, similar to  and  features.

In the absence of , the hypervisor default (on) is used. 
without the state attribute would be the same as  for
backwards compatibility. And of course  disables hap.

Signed-off-by: Jim Fehlig <jfeh...@suse.com>
---
 docs/formatdomain.html.in | 6 --
 docs/schemas/domaincommon.rng | 6 +-
 src/conf/domain_conf.c| 4 ++--
 3 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 5016772..c06bcf3 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -1494,8 +1494,10 @@
   Interrupt) for the guest.
   
   hap
-  Enable use of Hardware Assisted Paging if available in
-the hardware.
+  Depending on the state attribute (values 
on,
+off) enable or disable use of Hardware Assisted Paging.
+The default is on if the hypervisor detects availability
+of Hardware Assisted Paging.
   
   viridian
   Enable Viridian hypervisor extensions for paravirtualizing
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 67af93a..dd6e93a 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -4108,7 +4108,11 @@
   
   
 
-  
+  
+
+  
+
+  
 
   
   
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 79758d4..714bbfc 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -15296,7 +15296,6 @@ virDomainDefParseXML(xmlDocPtr xml,
 /* fallthrough */
 case VIR_DOMAIN_FEATURE_ACPI:
 case VIR_DOMAIN_FEATURE_PAE:
-case VIR_DOMAIN_FEATURE_HAP:
 case VIR_DOMAIN_FEATURE_VIRIDIAN:
 case VIR_DOMAIN_FEATURE_PRIVNET:
 case VIR_DOMAIN_FEATURE_HYPERV:
@@ -15321,6 +15320,7 @@ virDomainDefParseXML(xmlDocPtr xml,
 ctxt->node = node;
 break;
 
+case VIR_DOMAIN_FEATURE_HAP:
 case VIR_DOMAIN_FEATURE_PMU:
 case VIR_DOMAIN_FEATURE_PVSPINLOCK:
 case VIR_DOMAIN_FEATURE_VMPORT:
@@ -22043,7 +22043,6 @@ virDomainDefFormatInternal(virDomainDefPtr def,
 switch ((virDomainFeature) i) {
 case VIR_DOMAIN_FEATURE_ACPI:
 case VIR_DOMAIN_FEATURE_PAE:
-case VIR_DOMAIN_FEATURE_HAP:
 case VIR_DOMAIN_FEATURE_VIRIDIAN:
 case VIR_DOMAIN_FEATURE_PRIVNET:
 switch ((virTristateSwitch) def->features[i]) {
@@ -22065,6 +22064,7 @@ virDomainDefFormatInternal(virDomainDefPtr def,
 
 break;
 
+case VIR_DOMAIN_FEATURE_HAP:
 case VIR_DOMAIN_FEATURE_PMU:
 case VIR_DOMAIN_FEATURE_PVSPINLOCK:
 case VIR_DOMAIN_FEATURE_VMPORT:
-- 
2.1.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH V2 3/4] Xen drivers: show hap enabled by default in capabilities

[Jim Fehlig]

Hardware Assisted Paging is enabled by default in Xen. Change
the capabilities output to reflect this.

Signed-off-by: Jim Fehlig <jfeh...@suse.com>
---
 src/libxl/libxl_conf.c   | 2 +-
 src/xen/xen_hypervisor.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 93c943b..6efd9b5 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -493,7 +493,7 @@ libxlCapsInitGuests(libxl_ctx *ctx, virCapsPtr caps)
 
 if (virCapabilitiesAddGuestFeature(guest,
"hap",
-   0,
+   1,
1) == NULL)
 return -1;
 }
diff --git a/src/xen/xen_hypervisor.c b/src/xen/xen_hypervisor.c
index c1834cb..fc9e1c6 100644
--- a/src/xen/xen_hypervisor.c
+++ b/src/xen/xen_hypervisor.c
@@ -2206,7 +2206,7 @@ xenHypervisorBuildCapabilities(virConnectPtr conn, 
virArch hostarch,
 if ((hv_major == 3 && hv_minor >= 3) || (hv_major > 3))
 if (virCapabilitiesAddGuestFeature(guest,
"hap",
-   false,
+   true,
true) == NULL)
 goto no_memory;
 
-- 
2.1.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [libvirt] [PATCH V2 4/4] libxl: add support for rbd qdisk

[Jim Fehlig]

On 02/22/2016 07:35 AM, Ján Tomko wrote:
> On Wed, Feb 17, 2016 at 05:33:45PM -0700, Jim Fehlig wrote:
>> xl/libxl already supports qemu's network-based block backends
>> such as nbd and rbd. libvirt has supported configuring such
>> s for long time too. This patch adds support for rbd
>> disks in the libxl driver by generating a rbd device URL from
>> the virDomainDiskDef object. The URL is passed to libxl via the
>> pdev_path field of libxl_device_disk struct. libxl then passes
>> the URL to qemu for cosumption by the rbd backend.
>>
>> Signed-off-by: Jim Fehlig <jfeh...@suse.com>
>> ---
>>  src/libxl/libxl_conf.c | 192 
>> -
>>  1 file changed, 191 insertions(+), 1 deletion(-)
>>
> ACK with the whitespace fix.
>
>> +
>> +static int
>> +libxlMakeNetworkDiskSrc(virStorageSourcePtr src, char **srcstr)
>> +{
>> +virConnectPtr conn = NULL;
>> +char *secret = NULL;
>> +char *username = NULL;
>> +int ret = -1;
>> +
>> +*srcstr = NULL;
>> +if (src->auth && src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD) {
>> +const char *protocol = 
>> virStorageNetProtocolTypeToString(src->protocol);
>> +
>> +username = src->auth->username;
>> +if (!(conn = virConnectOpen("xen:///system")))
>> +goto cleanup;
>> +
> Opening a connection feels out of place in this function, but I see it's
> already done for NICs.
>
> It would be nice to reuse it as is done in the qemu driver.

Heh, this has turned out to be a PITA. Something like the attached patch works,
but it still has the virConnectOpen in the libxlBuildDomainConfig call path. I
would prefer to use the virConnect object associated with the request, instead
of opening one in this code. I have some old, dusty patches (originally started
by danpb) that provide unit tests for libxlBuildDomainConfig. E.g. domXML ->
virDomainDef -> libxl_domain_config -> json representation -> compare with
expected json. Code in this path attempting to open "xen:///" connections is
likely to break many 'make check' setups :-).

But fixing this is not easy given the current code structure. There is one place
in particular that is rather troublesome - reboot. libxlDomainStart, and hence
libxlBuildDomainConfig, are called when a reboot event is received from libxl
(e.g. 'shutdown -r now' within a VM). As you might guess, there is no virConnect
object associated with such request. The code needs reworked quite a bit to
handle accessing a virConnect object while building a libxl_domain_config
object. I'll work on this when dusting off the unit test patches, although it is
not high in my queue. Maybe it would make a good libvirt GSoC project :-).

Regards,
Jim


>From 869c76c029390ebb6e2b89b3bb0ccf9ac4610806 Mon Sep 17 00:00:00 2001
From: Jim Fehlig <jfeh...@suse.com>
Date: Tue, 23 Feb 2016 16:20:29 -0700
Subject: [PATCH] libxl: use a common virConnect object

Depending on the type of NIC or disk, libxlMakeNic and libxlMakeDisk
may need a virConnect object, e.g. to obtain a port from the network
driver or a secret from the secret driver. Instead of opening a
virConnect object in each of these functions, have the callers provide
the object.

This approach provides benefits over opening individual virConnect
objects, e.g. already fixing a virConnect unref bug in libxlMakeNic.
The downside is changing several functions to include a virConnectPtr
parameter.

Signed-off-by: Jim Fehlig <jfeh...@suse.com>
---
 src/libxl/libxl_conf.c   | 71 ++--
 src/libxl/libxl_conf.h   |  7 +++--
 src/libxl/libxl_driver.c | 43 ++---
 3 files changed, 71 insertions(+), 50 deletions(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index a109bb5..5d5c85e 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1071,9 +1071,10 @@ libxlMakeNetworkDiskSrcStr(virStorageSourcePtr src,
 }
 
 static int
-libxlMakeNetworkDiskSrc(virStorageSourcePtr src, char **srcstr)
+libxlMakeNetworkDiskSrc(virConnectPtr conn,
+virStorageSourcePtr src,
+char **srcstr)
 {
-virConnectPtr conn = NULL;
 char *secret = NULL;
 char *username = NULL;
 int ret = -1;
@@ -1083,9 +1084,6 @@ libxlMakeNetworkDiskSrc(virStorageSourcePtr src, char **srcstr)
 const char *protocol = virStorageNetProtocolTypeToString(src->protocol);
 
 username = src->auth->username;
-if (!(conn = virConnectOpen("xen:///system")))
-goto cleanup;
-
 if (!(secret = libxlGetSecretString(conn,
 protocol,

Re: [Xen-devel] [libvirt] [PATCH V2 4/4] libxl: add support for rbd qdisk

[Jim Fehlig]

On 02/22/2016 07:35 AM, Ján Tomko wrote:
> On Wed, Feb 17, 2016 at 05:33:45PM -0700, Jim Fehlig wrote:
>> xl/libxl already supports qemu's network-based block backends
>> such as nbd and rbd. libvirt has supported configuring such
>> s for long time too. This patch adds support for rbd
>> disks in the libxl driver by generating a rbd device URL from
>> the virDomainDiskDef object. The URL is passed to libxl via the
>> pdev_path field of libxl_device_disk struct. libxl then passes
>> the URL to qemu for cosumption by the rbd backend.
>>
>> Signed-off-by: Jim Fehlig <jfeh...@suse.com>
>> ---
>>  src/libxl/libxl_conf.c | 192 
>> -
>>  1 file changed, 191 insertions(+), 1 deletion(-)
>>
> ACK with the whitespace fix.

Hi Jan,

Sorry I missed your comments before pushing this series. I'll address them in a
follow-up. Thanks for your time reviewing these changes!

Regards,
Jim


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH V2 0/4] libxl: support qemu's network-based block backends

[Jim Fehlig]

On 02/18/2016 03:51 AM, Ian Campbell wrote:
> On Wed, 2016-02-17 at 17:33 -0700, Jim Fehlig wrote:
>> xl/libxl already supports qemu's network-based block backends
>> such as nbd and rbd. libvirt has supported configuring network
>> disks for long time too. This series marries the two in the
>> libxl driver and in the xl<->xml converter. Only rbd supported
>> is added in this series. Support for other backends such as nbd
>> and iscsi can be added as a follow-up improvement.
> All looks good to me, so FWIW:
> Acked-by: Ian Campbell <ian.campb...@citrix.com>

Thanks. I'm going to push this series. If we ever decide to extend
libxl_device_disk in libxl, corresponding changes in libvirt can be done with
LIBXL_HAVE_.

Regards,
Jim


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 3/3] docs: add more info about target= in disk config

[Jim Fehlig]

On 02/19/2016 10:23 AM, Ian Jackson wrote:
> Jim Fehlig writes ("[PATCH 3/3] docs: add more info about target= in disk 
> config"):
>> target= in disk config can be used to convey arbitrary
>> configuration information to backends. Add a bit more info
>> to xl-disk-configuration.txt to clarify this, including some
>> simple nbd and rbd qdisk configurations.
>> ---
>>  docs/misc/xl-disk-configuration.txt | 10 +-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/docs/misc/xl-disk-configuration.txt 
>> b/docs/misc/xl-disk-configuration.txt
>> index 29f6ddb..0918fb8 100644
>> --- a/docs/misc/xl-disk-configuration.txt
>> +++ b/docs/misc/xl-disk-configuration.txt
>> @@ -75,7 +75,15 @@ Special syntax:
>> the target was already specified as a positional parameter.  This
>> is the only way to specify a target string containing metacharacters
>> such as commas and (in some cases) colons, which would otherwise be
>> -   misinterpreted.
>> +   misinterpreted. Meta-information in a target string can be used to
>> +   specify configuration information for a qdisk block backend. For
>> +   example the nbd and rbd qdisk block backends can be configured with
>> +
>> + target=nbd:192.168.0.1:
>> + target=rbd:pool/image:mon_host=192.186.0.1\\:6789
>> +
>> +   Note the use of double backslash ('\\') for metacharacters that need
>> +   escaped.
> I'm not entirely comfortable with documenting this as supported.
> The difficulties I see are:
>
>
> In the usual configuration, libxl decides for itself what (libxl)
> backend to use.  Different versions of libxl might make different
> choices, so a configuration that works with one version of libxl might
> not work with another.  That's fine for an undocumented feature but
> not so good if it's actually advertised.  At the very least the docs
> need to say that to rely on this you must specify backend=qdisk.

The text I added in this patch states that meta-information can be used with
qdisk. I didn't go as far as saying _only_ qdisk, since other backends might
interpret such meta-information too. But I can add that if we decide to go with
this doc patch.

>
>
> And this is a layering violation, or rather a violation of the
> expected semantics of the target string.
>
> I think it would be much better to support nbd and rbd explicitly in
> libxl.  Maybe we should have a "protocol=" parameter, so you could
> write something like this:
>disk=["vdev=xvda, protocol=nbd, target=192.168.0.1:"]
>
> This would allow libxl to make better choices about backends, even if
> right now all it does is force the use of the qemu backend and pass
> the target string to qemu.

I agree with your suggestion, which is why I took that approach in the original
RFC post

http://lists.xenproject.org/archives/html/xen-devel/2016-01/msg03184.html

Did you see the doc and IDL RFC patch attached to that post? IMO, we need more
than just protocol. An rbd device configuration can include a pool/volume name,
multiple servers, auth type, auth username, and auth passwd/data. E.g.

  disk = [ 'vdev=xvda, backendtype=qdisk, backendprotocol=rbd,
server=192.168.0.1:, server=192.168.0.2:, auth=joe:joes-secret,
target=some-pool/some-image' ]

>
>
> Finally, if this is actually true, it is a bug.  The specification
> (docs/misc/xl-disk-configuration.txt) says:
>
>   Description:   Block device or image file path.  When this is
>  used as a path, /dev will be prepended
>  if the path doesn't start with a '/'.

That is not true. I've successfully used the following disk config

  disk = [ "vdev=xvdb, backendtype=qdisk,
   
target=rbd:libvirtpool/image:auth_supported=none:mon_host=192.168.0.1\\:6789\\;192.168.0.2\\:6789\\;192.168.0.3\\:6789"
]

disk = [ "vdev=xvdb, backendtype=qdisk, target=nbd:192.168.0.1:" ]

I wouldn't call those targets "image file paths", but they don't start with a
'/' and none is prepended.

> See also what is said under `script='.

Do you mean how the 

Re: [Xen-devel] [PATCH] docs: document handling of metacharacter escape in xl disk format

[Jim Fehlig]

On 02/19/2016 03:14 AM, Ian Campbell wrote:
> On Thu, 2016-02-18 at 15:44 -0700, Jim Fehlig wrote:
>> Ian Campbell wrote:
>>> Signed-off-by: Ian Campbell <ian.campb...@citrix.com>
>>> Cc: Jim Fehlig <jfeh...@suse.com>
>>> ---
>>>  docs/misc/xl-disk-configuration.txt | 18 ++
>>>  1 file changed, 18 insertions(+)
>>>
>>> diff --git a/docs/misc/xl-disk-configuration.txt b/docs/misc/xl-disk-
>>> configuration.txt
>>> index 6a2118d..a03ad10 100644
>>> --- a/docs/misc/xl-disk-configuration.txt
>>> +++ b/docs/misc/xl-disk-configuration.txt
>>> @@ -48,6 +48,24 @@ positionally or explicitly).
>>>  
>>>  Whitespace may appear before each parameter and will be ignored.
>>>  
>>> +Metacharacters in a  may be escaped using a backslash:
>>> +
>>> +Escape  HEX Description
>>> +--  --- ---
>>> +\a  0x07Bell
>>> +\b  0x08Backspace
>>> +\t  0x09Horizontal Tab
>>> +\n  0x0ANew Line / Line Feed
>>> +\f  0x0CForm Feed
>>> +\r  0x0DCarriage Return
>>> +\v  0x0BVertical Tab
>>> +\"  0x22A literal double quote
>>> +\'  0x27A literal single quote
>>> +\\  0x5CA literal backslash
>>> +\xXXCharacter XX in hexadecimal
>>> +\OOOCharacter OOO in octal
>> Do you know how any of these would be useful in a diskspec? I guess I'm
>> struggling to understand when a 'Bell' would be needed :-).
> I've not got a clue -- these are just all the ones handled by
> xlu__cfgl_dequote, perhaps just for completeness?

Maybe this code was copied from elsewhere and the unneeded/unused escapes were
not removed. Regardless, it's probably unwise to remove things like bell, form
feed, and vertical tab now.

>
> This is applied to all strings in a cfg file, not just diskspecs, 

Ah, right. And as such, I agree with your follow-up comment that this info
should then be in xl.cfg(5).

Regards,
Jim

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] docs: document handling of metacharacter escape in xl disk format

[Jim Fehlig]

Ian Campbell wrote:
> Signed-off-by: Ian Campbell <ian.campb...@citrix.com>
> Cc: Jim Fehlig <jfeh...@suse.com>
> ---
>  docs/misc/xl-disk-configuration.txt | 18 ++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/docs/misc/xl-disk-configuration.txt 
> b/docs/misc/xl-disk-configuration.txt
> index 6a2118d..a03ad10 100644
> --- a/docs/misc/xl-disk-configuration.txt
> +++ b/docs/misc/xl-disk-configuration.txt
> @@ -48,6 +48,24 @@ positionally or explicitly).
>  
>  Whitespace may appear before each parameter and will be ignored.
>  
> +Metacharacters in a  may be escaped using a backslash:
> +
> +Escape  HEX Description
> +--  --- ---
> +\a  0x07Bell
> +\b  0x08Backspace
> +\t  0x09Horizontal Tab
> +\n  0x0ANew Line / Line Feed
> +\f  0x0CForm Feed
> +\r  0x0DCarriage Return
> +\v  0x0BVertical Tab
> +\"  0x22A literal double quote
> +\'  0x27A literal single quote
> +\\  0x5CA literal backslash
> +\xXXCharacter XX in hexadecimal
> +\OOOCharacter OOO in octal

Do you know how any of these would be useful in a diskspec? I guess I'm
struggling to understand when a 'Bell' would be needed :-).

Regards,
Jim

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 0/4] libxl: support qemu's network-based block backends

[Jim Fehlig]

On 02/17/2016 03:24 AM, Ian Campbell wrote:
> On Tue, 2016-02-16 at 14:45 -0700, Jim Fehlig wrote:
>> xl/libxl already supports qemu's network-based block backends
>> such as nbd and rbd. libvirt has supported configuring network
>> disks for long time too. This series marries the two in the
>> libxl driver and in the xl<->xml converter. Only rbd supported
>> is added in this series. Support for other backends such as nbd
>> and iscsi can be added as a follow-up improvement.
> This all looks sensible to me, FWIW.

Thanks for taking a look!

>
> One question, in patch 3's commit log should the example be double escaping
> the \\ or not? Based on your updates to $xen/docs/misc/xl-disk-
> configuration.txt (posted separately on xen-devel) I had expected they
> would.

Yes, you are correct. The test and conversion code in patch 3 is wrong in that
regard too. I've fixed it in V2.

Regards,
Jim


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH V2 2/4] xenconfig: produce key=value disk config syntax in xl formatter

[Jim Fehlig]

The most formal form of xl disk configuration uses key=value
syntax to define each configuration item, e.g.

format=raw, vdev=xvda, access=rw, backendtype=phy, target=disksrc

Change the xl disk formatter to produce this syntax, which allows
target= to contain meta info needed to setup a network-based
disksrc (e.g. rbd, nbd, iscsi). For details on xl disk config
format, see  $xen-src/docs/misc/xl-disk-configuration.txt

Update the disk config in the tests to use the formal syntax.
But add tests to ensure disks specified with the positional
parameter syntax are correctly converted to  XML.

Signed-off-by: Jim Fehlig <jfeh...@suse.com>
---
 src/xenconfig/xen_xl.c | 27 ++-
 .../test-disk-positional-parms-full.cfg| 26 +++
 .../test-disk-positional-parms-full.xml| 54 ++
 .../test-disk-positional-parms-partial.cfg | 26 +++
 .../test-disk-positional-parms-partial.xml | 54 ++
 .../test-fullvirt-direct-kernel-boot.cfg   |  2 +-
 tests/xlconfigdata/test-fullvirt-multiusb.cfg  |  2 +-
 tests/xlconfigdata/test-new-disk.cfg   |  2 +-
 tests/xlconfigdata/test-paravirt-cmdline.cfg   |  2 +-
 tests/xlconfigdata/test-paravirt-maxvcpus.cfg  |  2 +-
 tests/xlconfigdata/test-spice-features.cfg |  2 +-
 tests/xlconfigdata/test-spice.cfg  |  2 +-
 tests/xlconfigdata/test-vif-rate.cfg   |  2 +-
 tests/xlconfigtest.c   |  2 +
 14 files changed, 186 insertions(+), 19 deletions(-)

diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c
index be194e3..f3e8b55 100644
--- a/src/xenconfig/xen_xl.c
+++ b/src/xenconfig/xen_xl.c
@@ -587,9 +587,8 @@ xenFormatXLDisk(virConfValuePtr list, virDomainDiskDefPtr 
disk)
 int format = virDomainDiskGetFormat(disk);
 const char *driver = virDomainDiskGetDriver(disk);
 
-/* target */
-virBufferAsprintf(, "%s,", src);
 /* format */
+virBufferAddLit(, "format=");
 switch (format) {
 case VIR_STORAGE_FILE_RAW:
 virBufferAddLit(, "raw,");
@@ -609,31 +608,37 @@ xenFormatXLDisk(virConfValuePtr list, virDomainDiskDefPtr 
disk)
 }
 
 /* device */
-virBufferAdd(, disk->dst, -1);
-
-virBufferAddLit(, ",");
+virBufferAsprintf(, "vdev=%s,", disk->dst);
 
+/* access */
+virBufferAddLit(, "access=");
 if (disk->src->readonly)
-virBufferAddLit(, "r,");
+virBufferAddLit(, "ro,");
 else if (disk->src->shared)
 virBufferAddLit(, "!,");
 else
-virBufferAddLit(, "w,");
+virBufferAddLit(, "rw,");
 if (disk->transient) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("transient disks not supported yet"));
 goto cleanup;
 }
 
+/* backendtype */
+virBufferAddLit(, "backendtype=");
 if (STREQ_NULLABLE(driver, "qemu"))
-virBufferAddLit(, "backendtype=qdisk");
+virBufferAddLit(, "qdisk,");
 else if (STREQ_NULLABLE(driver, "tap"))
-virBufferAddLit(, "backendtype=tap");
+virBufferAddLit(, "tap,");
 else if (STREQ_NULLABLE(driver, "phy"))
-virBufferAddLit(, "backendtype=phy");
+virBufferAddLit(, "phy,");
 
+/* devtype */
 if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM)
-virBufferAddLit(, ",devtype=cdrom");
+virBufferAddLit(, "devtype=cdrom,");
+
+/* target */
+virBufferAsprintf(, "target=%s", src);
 
 if (virBufferCheckError() < 0)
 goto cleanup;
diff --git a/tests/xlconfigdata/test-disk-positional-parms-full.cfg 
b/tests/xlconfigdata/test-disk-positional-parms-full.cfg
new file mode 100644
index 000..026e451
--- /dev/null
+++ b/tests/xlconfigdata/test-disk-positional-parms-full.cfg
@@ -0,0 +1,26 @@
+name = "XenGuest2"
+uuid = "c7a5fdb2-cdaf-9455-926a-d65c16db1809"
+maxmem = 579
+memory = 394
+vcpus = 1
+pae = 1
+acpi = 1
+apic = 1
+hap = 0
+viridian = 0
+rtc_timeoffset = 0
+localtime = 0
+on_poweroff = "destroy"
+on_reboot = "restart"
+on_crash = "restart"
+device_model = "/usr/lib/xen/bin/qemu-dm"
+sdl = 0
+vnc = 1
+vncunused = 1
+vnclisten = "127.0.0.1"
+vif = [ "mac=00:16:3e:66:92:9c,bridge=xenbr1,script=vif-bridge,model=e1000" ]
+parallel = "none"
+serial = "none"
+builder = "hvm"
+boot = "d"
+disk = [ "/dev/HostVG/XenGuest2,raw,hda,rw,backendtype=phy", 
"/var/lib/libvirt/images/XenGuest2-home,qcow2,hdb,rw", 
"/root/boot.iso,raw,hdc,ro,devtype=cdrom" ]
diff -

[Xen-devel] [PATCH V2 0/4] libxl: support qemu's network-based block backends

[Jim Fehlig]

xl/libxl already supports qemu's network-based block backends
such as nbd and rbd. libvirt has supported configuring network
disks for long time too. This series marries the two in the
libxl driver and in the xl<->xml converter. Only rbd supported
is added in this series. Support for other backends such as nbd
and iscsi can be added as a follow-up improvement.

Patch 1 is super trivial and contains no functional changes.

Patch 2 changes the xl disk configuration produced by the
xml->xl converter to use the formal key=value syntax described
in xl-disk-configuration.txt.

Patch 3 adds support for converting rbd info between xl and xml
config formats.

Patch 4 adds support for rbd disks in the libxl driver.

In V2:
Change commit msg, test, and code in patch3 to escape literal
backslashes with a backslash.

Jim Fehlig (4):
  xenconfig: replace text 'xm' with 'xl' in xlconfigtest
  xenconfig: produce key=value disk config syntax in xl formatter
  xenconfig: support xl<->xml conversion of rbd disk devices
  libxl: add support for rbd qdisk

 src/libxl/libxl_conf.c | 192 -
 src/xenconfig/xen_xl.c | 176 +--
 .../test-disk-positional-parms-full.cfg|  26 +++
 .../test-disk-positional-parms-full.xml|  54 ++
 .../test-disk-positional-parms-partial.cfg |  26 +++
 .../test-disk-positional-parms-partial.xml |  54 ++
 .../test-fullvirt-direct-kernel-boot.cfg   |   2 +-
 tests/xlconfigdata/test-fullvirt-multiusb.cfg  |   2 +-
 tests/xlconfigdata/test-new-disk.cfg   |   2 +-
 tests/xlconfigdata/test-paravirt-cmdline.cfg   |   2 +-
 tests/xlconfigdata/test-paravirt-maxvcpus.cfg  |   2 +-
 tests/xlconfigdata/test-rbd-multihost-noauth.cfg   |  26 +++
 tests/xlconfigdata/test-rbd-multihost-noauth.xml   |  51 ++
 tests/xlconfigdata/test-spice-features.cfg |   2 +-
 tests/xlconfigdata/test-spice.cfg  |   2 +-
 tests/xlconfigdata/test-vif-rate.cfg   |   2 +-
 tests/xlconfigtest.c   |  37 ++--
 17 files changed, 618 insertions(+), 40 deletions(-)
 create mode 100644 tests/xlconfigdata/test-disk-positional-parms-full.cfg
 create mode 100644 tests/xlconfigdata/test-disk-positional-parms-full.xml
 create mode 100644 tests/xlconfigdata/test-disk-positional-parms-partial.cfg
 create mode 100644 tests/xlconfigdata/test-disk-positional-parms-partial.xml
 create mode 100644 tests/xlconfigdata/test-rbd-multihost-noauth.cfg
 create mode 100644 tests/xlconfigdata/test-rbd-multihost-noauth.xml

-- 
2.1.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH V2 1/4] xenconfig: replace text 'xm' with 'xl' in xlconfigtest

[Jim Fehlig]

While at it, improve a few comments. No functional change.

Signed-off-by: Jim Fehlig <jfeh...@suse.com>
---
 tests/xlconfigtest.c | 34 +++---
 1 file changed, 19 insertions(+), 15 deletions(-)

diff --git a/tests/xlconfigtest.c b/tests/xlconfigtest.c
index 4b2f28f..aa53ed8 100644
--- a/tests/xlconfigtest.c
+++ b/tests/xlconfigtest.c
@@ -1,5 +1,5 @@
 /*
- * xlconfigtest.c: Test backend for xl_internal config file handling
+ * xlconfigtest.c: Test xl.cfg(5) <-> domXML config conversions
  *
  * Copyright (C) 2007, 2010-2011, 2014 Red Hat, Inc.
  * Copyright (c) 2015 SUSE LINUX Products GmbH, Nuernberg, Germany.
@@ -42,20 +42,22 @@
 
 static virCapsPtr caps;
 static virDomainXMLOptionPtr xmlopt;
+
 /*
- * parses the xml, creates a domain def and compare with equivalent xm config
+ * Parses domXML to virDomainDef object, which is then converted to xl.cfg(5)
+ * config and compared with expected config.
  */
 static int
-testCompareParseXML(const char *xmcfg, const char *xml)
+testCompareParseXML(const char *xlcfg, const char *xml)
 {
-char *gotxmcfgData = NULL;
+char *gotxlcfgData = NULL;
 virConfPtr conf = NULL;
 virConnectPtr conn = NULL;
 int wrote = 4096;
 int ret = -1;
 virDomainDefPtr def = NULL;
 
-if (VIR_ALLOC_N(gotxmcfgData, wrote) < 0)
+if (VIR_ALLOC_N(gotxlcfgData, wrote) < 0)
 goto fail;
 
 conn = virGetConnect();
@@ -73,17 +75,17 @@ testCompareParseXML(const char *xmcfg, const char *xml)
 if (!(conf = xenFormatXL(def, conn)))
 goto fail;
 
-if (virConfWriteMem(gotxmcfgData, , conf) < 0)
+if (virConfWriteMem(gotxlcfgData, , conf) < 0)
 goto fail;
-gotxmcfgData[wrote] = '\0';
+gotxlcfgData[wrote] = '\0';
 
-if (virtTestCompareToFile(gotxmcfgData, xmcfg) < 0)
+if (virtTestCompareToFile(gotxlcfgData, xlcfg) < 0)
 goto fail;
 
 ret = 0;
 
  fail:
-VIR_FREE(gotxmcfgData);
+VIR_FREE(gotxlcfgData);
 if (conf)
 virConfFree(conf);
 virDomainDefFree(def);
@@ -91,13 +93,15 @@ testCompareParseXML(const char *xmcfg, const char *xml)
 
 return ret;
 }
+
 /*
- * parses the xl config, develops domain def and compares with equivalent xm 
config
+ * Parses xl.cfg(5) config to virDomainDef object, which is then converted to
+ * domXML and compared to expected XML.
  */
 static int
-testCompareFormatXML(const char *xmcfg, const char *xml)
+testCompareFormatXML(const char *xlcfg, const char *xml)
 {
-char *xmcfgData = NULL;
+char *xlcfgData = NULL;
 char *gotxml = NULL;
 virConfPtr conf = NULL;
 int ret = -1;
@@ -107,10 +111,10 @@ testCompareFormatXML(const char *xmcfg, const char *xml)
 conn = virGetConnect();
 if (!conn) goto fail;
 
-if (virtTestLoadFile(xmcfg, ) < 0)
+if (virtTestLoadFile(xlcfg, ) < 0)
 goto fail;
 
-if (!(conf = virConfReadMem(xmcfgData, strlen(xmcfgData), 0)))
+if (!(conf = virConfReadMem(xlcfgData, strlen(xlcfgData), 0)))
 goto fail;
 
 if (!(def = xenParseXL(conf, caps, xmlopt)))
@@ -128,7 +132,7 @@ testCompareFormatXML(const char *xmcfg, const char *xml)
  fail:
 if (conf)
 virConfFree(conf);
-VIR_FREE(xmcfgData);
+VIR_FREE(xlcfgData);
 VIR_FREE(gotxml);
 virDomainDefFree(def);
 virObjectUnref(conn);
-- 
2.1.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH V2 4/4] libxl: add support for rbd qdisk

[Jim Fehlig]

xl/libxl already supports qemu's network-based block backends
such as nbd and rbd. libvirt has supported configuring such
s for long time too. This patch adds support for rbd
disks in the libxl driver by generating a rbd device URL from
the virDomainDiskDef object. The URL is passed to libxl via the
pdev_path field of libxl_device_disk struct. libxl then passes
the URL to qemu for cosumption by the rbd backend.

Signed-off-by: Jim Fehlig <jfeh...@suse.com>
---
 src/libxl/libxl_conf.c | 192 -
 1 file changed, 191 insertions(+), 1 deletion(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 48b77d2..5133299 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -46,6 +46,7 @@
 #include "libxl_conf.h"
 #include "libxl_utils.h"
 #include "virstoragefile.h"
+#include "base64.h"
 
 
 #define VIR_FROM_THIS VIR_FROM_LIBXL
@@ -920,17 +921,206 @@ libxlDomainGetEmulatorType(const virDomainDef *def)
 return ret;
 }
 
+static char *
+libxlGetSecretString(virConnectPtr conn,
+ const char *scheme,
+ bool encoded,
+ virStorageAuthDefPtr authdef,
+ virSecretUsageType secretUsageType)
+{
+size_t secret_size;
+virSecretPtr sec = NULL;
+char *secret = NULL;
+char uuidStr[VIR_UUID_STRING_BUFLEN];
+
+/* look up secret */
+switch (authdef->secretType) {
+case VIR_STORAGE_SECRET_TYPE_UUID:
+sec = virSecretLookupByUUID(conn, authdef->secret.uuid);
+virUUIDFormat(authdef->secret.uuid, uuidStr);
+break;
+case VIR_STORAGE_SECRET_TYPE_USAGE:
+sec = virSecretLookupByUsage(conn, secretUsageType,
+ authdef->secret.usage);
+break;
+}
+
+if (!sec) {
+if (authdef->secretType == VIR_STORAGE_SECRET_TYPE_UUID) {
+virReportError(VIR_ERR_NO_SECRET,
+   _("%s no secret matches uuid '%s'"),
+   scheme, uuidStr);
+} else {
+virReportError(VIR_ERR_NO_SECRET,
+   _("%s no secret matches usage value '%s'"),
+   scheme, authdef->secret.usage);
+}
+goto cleanup;
+}
+
+secret = (char *)conn->secretDriver->secretGetValue(sec, _size, 0,
+
VIR_SECRET_GET_VALUE_INTERNAL_CALL);
+if (!secret) {
+if (authdef->secretType == VIR_STORAGE_SECRET_TYPE_UUID) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("could not get value of the secret for "
+ "username '%s' using uuid '%s'"),
+   authdef->username, uuidStr);
+} else {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("could not get value of the secret for "
+ "username '%s' using usage value '%s'"),
+   authdef->username, authdef->secret.usage);
+}
+goto cleanup;
+}
+
+if (encoded) {
+char *base64 = NULL;
+
+base64_encode_alloc(secret, secret_size, );
+VIR_FREE(secret);
+if (!base64) {
+virReportOOMError();
+goto cleanup;
+}
+secret = base64;
+}
+
+ cleanup:
+virObjectUnref(sec);
+return secret;
+}
+
+static char *
+libxlMakeNetworkDiskSrcStr(virStorageSourcePtr src,
+   const char *username,
+   const char *secret)
+{
+char *ret = NULL;
+virBuffer buf = VIR_BUFFER_INITIALIZER;
+size_t i;
+
+switch ((virStorageNetProtocol) src->protocol) {
+case VIR_STORAGE_NET_PROTOCOL_NBD:
+case VIR_STORAGE_NET_PROTOCOL_HTTP:
+case VIR_STORAGE_NET_PROTOCOL_HTTPS:
+case VIR_STORAGE_NET_PROTOCOL_FTP:
+case VIR_STORAGE_NET_PROTOCOL_FTPS:
+case VIR_STORAGE_NET_PROTOCOL_TFTP:
+case VIR_STORAGE_NET_PROTOCOL_ISCSI:
+case VIR_STORAGE_NET_PROTOCOL_GLUSTER:
+case VIR_STORAGE_NET_PROTOCOL_SHEEPDOG:
+case VIR_STORAGE_NET_PROTOCOL_LAST:
+case VIR_STORAGE_NET_PROTOCOL_NONE:
+virReportError(VIR_ERR_NO_SUPPORT,
+   _("Unsupported network block protocol '%s'"),
+   virStorageNetProtocolTypeToString(src->protocol));
+goto cleanup;
+
+case VIR_STORAGE_NET_PROTOCOL_RBD:
+if (strchr(src->path, ':')) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("':' not allowed in RBD source volume name '%s'"),
+   src->path);
+goto cleanup;
+}
+
+virBufferStrcat(, "rbd:", src->path, NULL);
+
+   

[Xen-devel] [PATCH V2 3/4] xenconfig: support xl<->xml conversion of rbd disk devices

[Jim Fehlig]

The target= setting in xl disk configuration can be used to encode
meta info that is meaningful to a backend. Leverage this fact to
support qdisk network disk types such as rbd. E.g.  config
such as

   
 
 
   
   
   
 
 
 
   

can be converted to the following xl config (and vice versa)

  disk = [ "format=raw,vdev=hdb,access=rw,backendtype=qdisk,

target=rbd:pool/image:auth_supported=none:mon_host=mon1.example.org\\:6321\\;mon2.example.org\\:6322\\;mon3.example.org\\:6322"
 ]

Note that in xl disk config, a literal backslash in target= must
be escaped with a backslash. Conversion of  config is not
handled in this patch, but can be done in a follow-up patch.

Also add a test for the conversions.

Signed-off-by: Jim Fehlig <jfeh...@suse.com>
---

v2:
Change commit msg, test, and code to escape literal backslash
with a backslash.

 src/xenconfig/xen_xl.c   | 153 +--
 tests/xlconfigdata/test-rbd-multihost-noauth.cfg |  26 
 tests/xlconfigdata/test-rbd-multihost-noauth.xml |  51 
 tests/xlconfigtest.c |   1 +
 4 files changed, 224 insertions(+), 7 deletions(-)

diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c
index f3e8b55..585ef9b 100644
--- a/src/xenconfig/xen_xl.c
+++ b/src/xenconfig/xen_xl.c
@@ -246,6 +246,32 @@ xenParseXLSpice(virConfPtr conf, virDomainDefPtr def)
 return -1;
 }
 
+
+static int
+xenParseXLDiskSrc(virDomainDiskDefPtr disk, char *srcstr)
+{
+char *tmpstr = NULL;
+int ret = -1;
+
+if (STRPREFIX(srcstr, "rbd:")) {
+tmpstr = virStringReplace(srcstr, "", "\\");
+
+virDomainDiskSetType(disk, VIR_STORAGE_TYPE_NETWORK);
+disk->src->protocol = VIR_STORAGE_NET_PROTOCOL_RBD;
+ret = virStorageSourceParseRBDColonString(tmpstr, disk->src);
+} else {
+if (virDomainDiskSetSource(disk, srcstr) < 0)
+goto cleanup;
+
+ret = 0;
+}
+
+ cleanup:
+VIR_FREE(tmpstr);
+return ret;
+}
+
+
 /*
  * For details on xl disk config syntax, see
  * docs/misc/xl-disk-configuration.txt in the Xen sources.  The important
@@ -311,12 +337,12 @@ xenParseXLDisk(virConfPtr conf, virDomainDefPtr def)
 if (!(disk = virDomainDiskDefNew(NULL)))
 goto fail;
 
+if (xenParseXLDiskSrc(disk, libxldisk->pdev_path) < 0)
+goto fail;
+
 if (VIR_STRDUP(disk->dst, libxldisk->vdev) < 0)
 goto fail;
 
-if (virDomainDiskSetSource(disk, libxldisk->pdev_path) < 0)
-goto fail;
-
 disk->src->readonly = !libxldisk->readwrite;
 disk->removable = libxldisk->removable;
 
@@ -358,7 +384,8 @@ xenParseXLDisk(virConfPtr conf, virDomainDefPtr def)
 case LIBXL_DISK_BACKEND_UNKNOWN:
 if (virDomainDiskSetDriver(disk, "qemu") < 0)
 goto fail;
-virDomainDiskSetType(disk, VIR_STORAGE_TYPE_FILE);
+if (virDomainDiskGetType(disk) == VIR_STORAGE_TYPE_NONE)
+virDomainDiskSetType(disk, VIR_STORAGE_TYPE_FILE);
 break;
 
 case LIBXL_DISK_BACKEND_TAP:
@@ -578,14 +605,115 @@ xenFormatXLOS(virConfPtr conf, virDomainDefPtr def)
 }
 
 
+static char *
+xenFormatXLDiskSrcNet(virStorageSourcePtr src)
+{
+char *ret = NULL;
+virBuffer buf = VIR_BUFFER_INITIALIZER;
+size_t i;
+
+switch ((virStorageNetProtocol) src->protocol) {
+case VIR_STORAGE_NET_PROTOCOL_NBD:
+case VIR_STORAGE_NET_PROTOCOL_HTTP:
+case VIR_STORAGE_NET_PROTOCOL_HTTPS:
+case VIR_STORAGE_NET_PROTOCOL_FTP:
+case VIR_STORAGE_NET_PROTOCOL_FTPS:
+case VIR_STORAGE_NET_PROTOCOL_TFTP:
+case VIR_STORAGE_NET_PROTOCOL_ISCSI:
+case VIR_STORAGE_NET_PROTOCOL_GLUSTER:
+case VIR_STORAGE_NET_PROTOCOL_SHEEPDOG:
+case VIR_STORAGE_NET_PROTOCOL_LAST:
+case VIR_STORAGE_NET_PROTOCOL_NONE:
+virReportError(VIR_ERR_NO_SUPPORT,
+   _("Unsupported network block protocol '%s'"),
+   virStorageNetProtocolTypeToString(src->protocol));
+goto cleanup;
+
+case VIR_STORAGE_NET_PROTOCOL_RBD:
+if (strchr(src->path, ':')) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("':' not allowed in RBD source volume name '%s'"),
+   src->path);
+goto cleanup;
+}
+
+virBufferStrcat(, "rbd:", src->path, NULL);
+
+virBufferAddLit(, ":auth_supported=none");
+
+if (src->nhosts > 0) {
+virBufferAddLit(, ":mon_host=");
+for (i = 0; i < src->nhosts; i++) {
+if (i)
+

Re: [Xen-devel] [PATCH 1/3] libxlu_cfg: reject unknown characters following '\'

[Jim Fehlig]

On 02/17/2016 03:11 AM, Ian Campbell wrote:
> On Wed, 2016-02-17 at 10:05 +, Ian Campbell wrote:
>> On Tue, 2016-02-16 at 20:54 -0700, Jim Fehlig wrote:
>>> When dequoting config strings in xlu__cfgl_dequote(), unknown
>>> characters following a '\', and the '\' itself, are discarded.
>>> E.g. a disk configuration string containing
>>>
>>>   rbd:pool/image:mon_host=192.168.0.100\:6789
>>>
>>> would be dequoted as
>>>
>>>   rbd:pool/image:mon_host=192.168.0.1006789
>>>
>>> Instead of discarding the '\' and unknown character, reject the
>>> string and set error to EINVAL.
>> Missing your S-o-b.
>>
>> Other than that:
>>
>>> +xlu__cfgl_lexicalerror(ctx, "invalid character after
>>> backlash "
>>> +   "in quoted string");
>> Please try where possible not to split string constants (so log messages
>> can more easily be grepped for).
> I see now that this parsing code is pretty liberally ignoring this advice
> already. So apart from the missing S-o-b this patch is

Your suggestion is a good one and is common throughout much of libxl, so I
pulled back the error string indentation in V2.

Regards,
Jim


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 3/3] docs: add more info about target= in disk config

[Jim Fehlig]

On 02/17/2016 03:10 AM, Ian Campbell wrote:
> On Tue, 2016-02-16 at 20:54 -0700, Jim Fehlig wrote:
>> target= in disk config can be used to convey arbitrary
>> configuration information to backends. Add a bit more info
>> to xl-disk-configuration.txt to clarify this, including some
>> simple nbd and rbd qdisk configurations.
> Missing S-o-b.
>
>> ---
>>  docs/misc/xl-disk-configuration.txt | 10 +-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/docs/misc/xl-disk-configuration.txt b/docs/misc/xl-disk-
>> configuration.txt
>> index 29f6ddb..0918fb8 100644
>> --- a/docs/misc/xl-disk-configuration.txt
>> +++ b/docs/misc/xl-disk-configuration.txt
>> @@ -75,7 +75,15 @@ Special syntax:
>> the target was already specified as a positional parameter.  This
>> is the only way to specify a target string containing metacharacters
>> such as commas and (in some cases) colons, which would otherwise be
>> -   misinterpreted.
>> +   misinterpreted. Meta-information in a target string can be used to
>> +   specify configuration information for a qdisk block backend. For
>> +   example the nbd and rbd qdisk block backends can be configured with
>> +
>> + target=nbd:192.168.0.1:
>> + target=rbd:pool/image:mon_host=192.186.0.1\\:6789
>> +
>> +   Note the use of double backslash ('\\') for metacharacters that need
>> +   escaped.
> "need to be escaped".
>
> However I wouldn't describe "\\" that way, I think I would say "note that \
> is used to escape metacharacters and therefore to get a literal backslash
> "\\" is required".

Agreed. I changed the text and sent a V2 of the series.

>
> The general concept of escaping metacharaters is not mentioned in this doc
> at all, i.e. there is no mention of which characters need such escaping nor
> of the various "special" codes (\t and \n etc), nor of the octal and hex
> escape codes. Maybe that's a topic for another patch though.

I could do that in a follow-up, but I have no clue what those mean or how they
are used.

Regards,
Jim


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel