Re: [libvirt] [PATCH V2] virconf: properly set the end of content

[Jim Fehlig]

Opps, sorry for missing the 'PATCH V2' subject-prefix.

Regards,
Jim

On 11/08/2017 04:01 PM, Jim Fehlig wrote:

There was a recent report of the xen-xl converter not handling
config files missing an ending newline

https://www.redhat.com/archives/libvir-list/2017-October/msg01353.html

Commit 3cc2a9e0 fixed a similar problem when parsing content of a
file but missed parsing in-memory content. But AFAICT, the better
fix is to properly set the end of the content when initializing the
virConfParserCtxt in virConfParse().

This commit reverts the part of 3cc2a9e0 that appends a newline to
files missing it, and fixes setting the end of content when
initializing virConfParserCtxt. A test is also added to check
parsing in-memory content missing an ending newline.

Signed-off-by: Jim Fehlig 
---
  src/util/virconf.c  | 13 ++
  tests/virconftest.c | 69 +
  2 files changed, 71 insertions(+), 11 deletions(-)

diff --git a/src/util/virconf.c b/src/util/virconf.c
index 39c2bd917..a82a509ca 100644
--- a/src/util/virconf.c
+++ b/src/util/virconf.c
@@ -705,7 +705,7 @@ virConfParse(const char *filename, const char *content, int 
len,
  
  ctxt.filename = filename;

  ctxt.base = ctxt.cur = content;
-ctxt.end = content + len - 1;
+ctxt.end = content + len;
  ctxt.line = 1;
  
  ctxt.conf = virConfCreate(filename, flags);

@@ -745,7 +745,7 @@ virConfReadFile(const char *filename, unsigned int flags)
  {
  char *content;
  int len;
-virConfPtr conf = NULL;
+virConfPtr conf;
  
  VIR_DEBUG("filename=%s", NULLSTR(filename));
  
@@ -757,17 +757,8 @@ virConfReadFile(const char *filename, unsigned int flags)

  if ((len = virFileReadAll(filename, MAX_CONFIG_FILE_SIZE, )) < 0)
  return NULL;
  
-if (len && len < MAX_CONFIG_FILE_SIZE && content[len - 1] != '\n') {

-VIR_DEBUG("appending newline to busted config file %s", filename);
-if (VIR_REALLOC_N(content, len + 2) < 0)
-goto cleanup;
-content[len++] = '\n';
-content[len] = '\0';
-}
-
  conf = virConfParse(filename, content, len, flags);
  
- cleanup:

  VIR_FREE(content);
  
  return conf;

diff --git a/tests/virconftest.c b/tests/virconftest.c
index a8b18bae0..3cf0df3ac 100644
--- a/tests/virconftest.c
+++ b/tests/virconftest.c
@@ -77,6 +77,72 @@ static int testConfRoundTrip(const void *opaque)
  }
  
  
+static int testConfMemoryNoNewline(const void *opaque ATTRIBUTE_UNUSED)

+{
+const char *srcdata = \
+"ullong = '123456789'\n" \
+"string = 'foo'\n" \
+"uint = 12345";
+
+virConfPtr conf = virConfReadString(srcdata, 0);
+int ret = -1;
+virConfValuePtr val;
+unsigned long long llvalue;
+char *str = NULL;
+int uintvalue;
+
+if (!conf)
+return -1;
+
+if (!(val = virConfGetValue(conf, "ullong")))
+goto cleanup;
+
+if (val->type != VIR_CONF_STRING)
+goto cleanup;
+
+if (virStrToLong_ull(val->str, NULL, 10, ) < 0)
+goto cleanup;
+
+if (llvalue != 123456789) {
+fprintf(stderr, "Expected '123' got '%llu'\n", llvalue);
+goto cleanup;
+}
+
+if (virConfGetValueType(conf, "string") !=
+VIR_CONF_STRING) {
+fprintf(stderr, "expected a string for 'string'\n");
+goto cleanup;
+}
+
+if (virConfGetValueString(conf, "string", ) < 0)
+goto cleanup;
+
+if (STRNEQ_NULLABLE(str, "foo")) {
+fprintf(stderr, "Expected 'foo' got '%s'\n", str);
+goto cleanup;
+}
+
+if (virConfGetValueType(conf, "uint") != VIR_CONF_ULLONG) {
+fprintf(stderr, "expected an unsigned long for 'uint'\n");
+goto cleanup;
+}
+
+if (virConfGetValueInt(conf, "uint", ) < 0)
+goto cleanup;
+
+if (uintvalue != 12345) {
+fprintf(stderr, "Expected 12345 got %ud\n", uintvalue);
+goto cleanup;
+}
+
+ret = 0;
+ cleanup:
+VIR_FREE(str);
+virConfFree(conf);
+return ret;
+}
+
+
  static int testConfParseInt(const void *opaque ATTRIBUTE_UNUSED)
  {
  const char *srcdata = \
@@ -414,6 +480,9 @@ mymain(void)
  if (virTestRun("no-newline", testConfRoundTrip, "no-newline") < 0)
  ret = -1;
  
+if (virTestRun("memory-no-newline", testConfMemoryNoNewline, NULL) < 0)

+ret = -1;
+
  if (virTestRun("int", testConfParseInt, NULL) < 0)
  ret = -1;
  



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

Re: [libvirt] [PATCH] virconf: properly set the end of content

[Jim Fehlig]

On 11/08/2017 01:56 AM, Daniel P. Berrange wrote:

On Tue, Nov 07, 2017 at 03:37:46PM -0700, Jim Fehlig wrote:

There was a recent report of the xen-xl converter not handling
config files missing an ending newline

https://www.redhat.com/archives/libvir-list/2017-October/msg01353.html

Commit 3cc2a9e0 fixed a similar problem when parsing content of a
file but missed parsing in-memory content. But AFAICT, the better
fix is to properly set the end of the content when initializing the
virConfParserCtxt in virConfParse().

This commit reverts the part of 3cc2a9e0 that appends a newline to
files missing it, and fixes setting the end of content when
initializing virConfParserCtxt.

Signed-off-by: Jim Fehlig 
---
  src/util/virconf.c | 13 ++---
  1 file changed, 2 insertions(+), 11 deletions(-)


Could you try to extend  tests/virconftest.c to cover in-memory
content for this newline scenario too.


I cooked up some content that failed without this change and passes with it. V2 
sent

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

Regards,
Jim

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

[libvirt] [PATCH] virconf: properly set the end of content

[Jim Fehlig]

There was a recent report of the xen-xl converter not handling
config files missing an ending newline

https://www.redhat.com/archives/libvir-list/2017-October/msg01353.html

Commit 3cc2a9e0 fixed a similar problem when parsing content of a
file but missed parsing in-memory content. But AFAICT, the better
fix is to properly set the end of the content when initializing the
virConfParserCtxt in virConfParse().

This commit reverts the part of 3cc2a9e0 that appends a newline to
files missing it, and fixes setting the end of content when
initializing virConfParserCtxt. A test is also added to check
parsing in-memory content missing an ending newline.

Signed-off-by: Jim Fehlig 
---
 src/util/virconf.c  | 13 ++
 tests/virconftest.c | 69 +
 2 files changed, 71 insertions(+), 11 deletions(-)

diff --git a/src/util/virconf.c b/src/util/virconf.c
index 39c2bd917..a82a509ca 100644
--- a/src/util/virconf.c
+++ b/src/util/virconf.c
@@ -705,7 +705,7 @@ virConfParse(const char *filename, const char *content, int 
len,
 
 ctxt.filename = filename;
 ctxt.base = ctxt.cur = content;
-ctxt.end = content + len - 1;
+ctxt.end = content + len;
 ctxt.line = 1;
 
 ctxt.conf = virConfCreate(filename, flags);
@@ -745,7 +745,7 @@ virConfReadFile(const char *filename, unsigned int flags)
 {
 char *content;
 int len;
-virConfPtr conf = NULL;
+virConfPtr conf;
 
 VIR_DEBUG("filename=%s", NULLSTR(filename));
 
@@ -757,17 +757,8 @@ virConfReadFile(const char *filename, unsigned int flags)
 if ((len = virFileReadAll(filename, MAX_CONFIG_FILE_SIZE, )) < 0)
 return NULL;
 
-if (len && len < MAX_CONFIG_FILE_SIZE && content[len - 1] != '\n') {
-VIR_DEBUG("appending newline to busted config file %s", filename);
-if (VIR_REALLOC_N(content, len + 2) < 0)
-goto cleanup;
-content[len++] = '\n';
-content[len] = '\0';
-}
-
 conf = virConfParse(filename, content, len, flags);
 
- cleanup:
 VIR_FREE(content);
 
 return conf;
diff --git a/tests/virconftest.c b/tests/virconftest.c
index a8b18bae0..3cf0df3ac 100644
--- a/tests/virconftest.c
+++ b/tests/virconftest.c
@@ -77,6 +77,72 @@ static int testConfRoundTrip(const void *opaque)
 }
 
 
+static int testConfMemoryNoNewline(const void *opaque ATTRIBUTE_UNUSED)
+{
+const char *srcdata = \
+"ullong = '123456789'\n" \
+"string = 'foo'\n" \
+"uint = 12345";
+
+virConfPtr conf = virConfReadString(srcdata, 0);
+int ret = -1;
+virConfValuePtr val;
+unsigned long long llvalue;
+char *str = NULL;
+int uintvalue;
+
+if (!conf)
+return -1;
+
+if (!(val = virConfGetValue(conf, "ullong")))
+goto cleanup;
+
+if (val->type != VIR_CONF_STRING)
+goto cleanup;
+
+if (virStrToLong_ull(val->str, NULL, 10, ) < 0)
+goto cleanup;
+
+if (llvalue != 123456789) {
+fprintf(stderr, "Expected '123' got '%llu'\n", llvalue);
+goto cleanup;
+}
+
+if (virConfGetValueType(conf, "string") !=
+VIR_CONF_STRING) {
+fprintf(stderr, "expected a string for 'string'\n");
+goto cleanup;
+}
+
+if (virConfGetValueString(conf, "string", ) < 0)
+goto cleanup;
+
+if (STRNEQ_NULLABLE(str, "foo")) {
+fprintf(stderr, "Expected 'foo' got '%s'\n", str);
+goto cleanup;
+}
+
+if (virConfGetValueType(conf, "uint") != VIR_CONF_ULLONG) {
+fprintf(stderr, "expected an unsigned long for 'uint'\n");
+goto cleanup;
+}
+
+if (virConfGetValueInt(conf, "uint", ) < 0)
+goto cleanup;
+
+if (uintvalue != 12345) {
+fprintf(stderr, "Expected 12345 got %ud\n", uintvalue);
+goto cleanup;
+}
+
+ret = 0;
+ cleanup:
+VIR_FREE(str);
+virConfFree(conf);
+return ret;
+}
+
+
 static int testConfParseInt(const void *opaque ATTRIBUTE_UNUSED)
 {
 const char *srcdata = \
@@ -414,6 +480,9 @@ mymain(void)
 if (virTestRun("no-newline", testConfRoundTrip, "no-newline") < 0)
 ret = -1;
 
+if (virTestRun("memory-no-newline", testConfMemoryNoNewline, NULL) < 0)
+ret = -1;
+
 if (virTestRun("int", testConfParseInt, NULL) < 0)
 ret = -1;
 
-- 
2.14.2

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

Re: [libvirt] [PATCH v3 0/5] nwfilter common object adjustments

[John Ferlan]

Ping?

Any brave souls out there?

Tks,

John

On 10/05/2017 07:32 PM, John Ferlan wrote:
> v2: https://www.redhat.com/archives/libvir-list/2017-July/msg00673.html
> (and a few pings along the way)
> 
> Don't think much survived from v2 - this is a fresh start anyway.
> Perhaps old patch 2 the same, but beyond that a different approach
> to remove recursive read/write locks and replace with using rwlock
> read/write where the write's are in very tight confines.
> 
> I've run the changes through avocado with success. There were some
> really strange deadlocks along the way - even causing libvirtd to
> go defunct. There's a lot of strange ways to use/access the nwfilters.
> 
> John Ferlan (5):
>   nwfilter: Add update locking to Initialization
>   nwfilter: Remove unnecessary UUID comparison bypass
>   nwfilter: Convert _virNWFilterObj to use virObjectRWLockable
>   nwfilter: Convert _virNWFilterObjList to use virObjectRWLockable
>   nwfilter: Remove need for nwfilterDriverLock in some API's
> 
>  src/conf/virnwfilterobj.c  | 555 
> +++--
>  src/conf/virnwfilterobj.h  |  11 +-
>  src/libvirt_private.syms   |   3 +-
>  src/nwfilter/nwfilter_driver.c |  77 +++--
>  src/nwfilter/nwfilter_gentech_driver.c |  11 +-
>  5 files changed, 433 insertions(+), 224 deletions(-)
> 

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

Re: [libvirt] [PATCH 3/4] qemu: Remove unnecessary virtio disk detach info.alias check

[John Ferlan]

On 10/20/2017 08:21 AM, Ján Tomko wrote:
> On Fri, Oct 20, 2017 at 08:03:29AM -0400, John Ferlan wrote:
>> Since qemuAssignDeviceDiskAlias checks whether the input info.alias
>> is already present, no need to check here as well.
>>
>> Signed-off-by: John Ferlan 
>> ---
>> src/qemu/qemu_hotplug.c | 6 ++
>> 1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>> index 51a7a68f84..9fbb3a0712 100644
>> --- a/src/qemu/qemu_hotplug.c
>> +++ b/src/qemu/qemu_hotplug.c
>> @@ -4447,10 +4447,8 @@
>> qemuDomainDetachVirtioDiskDevice(virQEMUDriverPtr driver,
>>     goto cleanup;
>>     }
>>
>> -    if (!detach->info.alias) {
>> -    if (qemuAssignDeviceDiskAlias(vm->def, detach,
>> priv->qemuCaps) < 0)
>> -    goto cleanup;
>> -    }
>> +    if (qemuAssignDeviceDiskAlias(vm->def, detach, priv->qemuCaps) < 0)
>> +    goto cleanup;
>>
> 
> All the calls assigning aliases in the Detach functions are
> unnecessary. At this point, all the domain's devices should have their
> aliases assigned. If by any case they do not, it is an error in other
> part of the libvirt code.
> 
> I was going to send patches cleaning these up, but I could not decide
> whether to report an error if we find a device without an alias,
> or to just quietly assume that an alias. And I did not want to conflict
> with Michal's series again.
> 
> Jan
> 

I cycled back to this - if the alias was NULL and we're using json, then
virJSONValueObjectAddVArgs will fail w/ "argument key 'id'
must not have null value"; however, the text command path would fail in
qemuMonitorEscapeArg as soon as @in is deref'd.

So quietly assuming could end badly, but then again only for the old non
json path.

So either we report an error or just do what I did and rebuilt up the
alias. Is there a preference?  IDC, either way.

Tks -

John
>>     qemuDomainMarkDeviceForRemoval(vm, >info);
>>
>> -- 
>> 2.13.6
>>
>> -- 
>> libvir-list mailing list
>> libvir-list@redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list

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

Re: [libvirt] [PATCH 0/7] x86: Rework KVM-defaults compat code, enable kvm_pv_unhalt by default

[Eduardo Habkost]

On Tue, Nov 07, 2017 at 12:21:16PM +0100, Paolo Bonzini wrote:
> On 14/10/2017 01:56, Eduardo Habkost wrote:
> > Now, I don't know yet what's the best default for a guest that
> > has CONFIG_PARAVIRT_SPINLOCK when it sees a host that supports
> > kvm_pv_unhalt.  But I'm arguing that it's the guest
> > responsibility to choose what to do when it detects such a host,
> > instead of expecting the host to hide features from the guest.
> > The guest and the guest administrator have more information to
> > choose what's best.
> > 
> > In other words, if exposing kvm_pv_unhalt on CPUID really makes
> > some guests behave poorly, can we fix the guests instead?
> 
> Waiman just did it. :)
> 
> https://marc.info/?l=linux-kernel=150972337909996

Thanks for the pointer.  I will resubmit the series for 2.12
after 2.11 is released.

-- 
Eduardo

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

Re: [libvirt] [PATCH 08/11] tools: Provide bash autompletion file

[Eric Blake]

On 11/08/2017 10:06 AM, Michal Privoznik wrote:

>> $ virsh complete some-command --arg partial
> 
> This is excatly what my patches are doing. With one tiny difference. In
> fact bash script calls:
> 
>   virsh complete "some-command --arg partial"
> 
> so that I can have cmdComplete which takes exactly one string argument.

Ouch. That difference matters, and I don't think you want to do that.

We don't want to reimplement shell parsing; if the user does:

virsh snapshot-create-as 'domain with space' 'name with space'
'description with space', then my approach feeds those three arguments
as is (the virsh parser doesn't have to undo any shell quoting), while
your approach HAS to over-quote the original command line, then
reimplement shell quoting to remove those quotes in virsh, in order to
reconstruct the original line in spite of being temporarily squashed
through one argument.

> Parsing of the string is then done within cmdComplete. I don't think
> that we have variable args in virsh for commands, do we?

Yes, we do.  See 'virsh echo' for an example of its use.

> 
> Correct. And again, my patches do this. For instance:
> 
>   virsh -r -c qemu+ssh://host/system domifaddr --domain
> 
> becomes:
> 
>   virsh -r -c qemu+ssh://host/system complete "domifaddr --domain"
> 
> And it's the 'complete' command that is responsible for bringing up a
> list of possible strings.

Ah, you are trying to put 'complete' after virsh-specific options (-r,
-c), but before the command (domifaddr).

And if properly implemented, you should be able to have:

virsh domif

show 'domifaddr' (and any other commands starting with 'domif') (by
calling "virsh complete -- domif"),

virsh domifaddr 

show both a list of domain names AND a list of --options accepted by
domifaddr (by calling "virsh complete -- domifaddr ''"),

virsh domifaddr --

show a list of long options for domifaddr (by calling "virsh complete --
domifaddr --") (yes, I'm specifically making sure the second '--'
doesn't get eaten by getopts, by inserting 'complete --' rather than
just 'complete'), and

virsh domifaddr a

show a filtered list of just domain names starting with 'a' (by calling
"virsh complete domifaddr a").

That is, the completions should be context-sensitive based on how much
of the command line is present prior to the , and should NOT
need the user to type an explicit --domain just to get domain completions.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



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

Re: [libvirt] [PATCH 08/11] tools: Provide bash autompletion file

[Michal Privoznik]

On 11/08/2017 04:53 PM, Eric Blake wrote:
> On 11/08/2017 09:09 AM, Michal Privoznik wrote:
>> On 11/08/2017 03:46 PM, Martin Kletzander wrote:
>>> On Tue, Nov 07, 2017 at 01:22:56PM +0100, Michal Privoznik wrote:
 Signed-off-by: Michal Privoznik 
> 
>>>
>>> When I see all the things you have to do here, wouldn't it be easier to
>>> have a
>>> virsh 'option' rather than a 'command' so that we don't have to parse
>>> anything
>>> twice and just circumvent the command execution in virsh itself?
>>
>> Not really. That would mean parsing the command line in cmdComplete.
>> Which again might be incomplete and thus yet more code would be needed.
>> I don't really see a problem with this approach - now that the bash
>> script is written.
>>
>>>   You
>>> would just
>>> run the same command with '-C' (for example) appended after the program
>>> name.
>>
>> Yeah, there are dozen of other approaches. I've chosen this one. I'm
>> failing to see why one is better than another one.
>>
> 
> [I haven't read this thread closely yet, just adding a drive-by comment]
> 
> Several years ago, when autocompletion was attempted (and failed) as a
> GSoC project, I had several ideas on how completion should work, and how
> it should be tested.
> 
> Ideally, we need a new virsh command 'complete', which takes varargs,
> and then performs completion based on those args.  So the bash script
> for completion for this scenario:
> 
> $ virsh some-command --arg partial
> 
> is as simple as having the completion function in bash call:
> 
> $ virsh complete some-command --arg partial

This is excatly what my patches are doing. With one tiny difference. In
fact bash script calls:

  virsh complete "some-command --arg partial"

so that I can have cmdComplete which takes exactly one string argument.
Parsing of the string is then done within cmdComplete. I don't think
that we have variable args in virsh for commands, do we?

> 
> The complete command in virsh should then know that it is performing
> completion on some-command, and do enough arg parsing to determine how
> to complete 'partial' in the current context of whatever argument
> some-command would be parsing at that point.
> 
> If a user in bash backs up the cursor and types  somewhere other
> than the end of the line, hopefully bash gives us enough hooks to call
> 'virsh complete args-up-to-TAB' by merely truncating whatever appears
> beyond the point where the user attempted tab completion.
> 
> ALL the completion logic lives in virsh, nothing in bash - all bash has
> to do is insert 'complete' and call into virsh.  That is, once you've
> written the bash script, it should NEVER need future modification,
> because any further improvements to completion will live in virsh
> (whether you use virsh in interactive mode or in batch mode from the shell).

Correct. And again, my patches do this. For instance:

  virsh -r -c qemu+ssh://host/system domifaddr --domain

becomes:

  virsh -r -c qemu+ssh://host/system complete "domifaddr --domain"

And it's the 'complete' command that is responsible for bringing up a
list of possible strings.

Michal

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

Re: [libvirt] [PATCH 08/11] tools: Provide bash autompletion file

[Eric Blake]

On 11/08/2017 09:09 AM, Michal Privoznik wrote:
> On 11/08/2017 03:46 PM, Martin Kletzander wrote:
>> On Tue, Nov 07, 2017 at 01:22:56PM +0100, Michal Privoznik wrote:
>>> Signed-off-by: Michal Privoznik 

>>
>> When I see all the things you have to do here, wouldn't it be easier to
>> have a
>> virsh 'option' rather than a 'command' so that we don't have to parse
>> anything
>> twice and just circumvent the command execution in virsh itself?
> 
> Not really. That would mean parsing the command line in cmdComplete.
> Which again might be incomplete and thus yet more code would be needed.
> I don't really see a problem with this approach - now that the bash
> script is written.
> 
>>   You
>> would just
>> run the same command with '-C' (for example) appended after the program
>> name.
> 
> Yeah, there are dozen of other approaches. I've chosen this one. I'm
> failing to see why one is better than another one.
> 

[I haven't read this thread closely yet, just adding a drive-by comment]

Several years ago, when autocompletion was attempted (and failed) as a
GSoC project, I had several ideas on how completion should work, and how
it should be tested.

Ideally, we need a new virsh command 'complete', which takes varargs,
and then performs completion based on those args.  So the bash script
for completion for this scenario:

$ virsh some-command --arg partial

is as simple as having the completion function in bash call:

$ virsh complete some-command --arg partial

The complete command in virsh should then know that it is performing
completion on some-command, and do enough arg parsing to determine how
to complete 'partial' in the current context of whatever argument
some-command would be parsing at that point.

If a user in bash backs up the cursor and types  somewhere other
than the end of the line, hopefully bash gives us enough hooks to call
'virsh complete args-up-to-TAB' by merely truncating whatever appears
beyond the point where the user attempted tab completion.

ALL the completion logic lives in virsh, nothing in bash - all bash has
to do is insert 'complete' and call into virsh.  That is, once you've
written the bash script, it should NEVER need future modification,
because any further improvements to completion will live in virsh
(whether you use virsh in interactive mode or in batch mode from the shell).

I don't know how much my original idea has carried over into your
proposal in this thread, but you may want to read the emails I posted
from the archives on the topic, for more ideas.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



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

Re: [libvirt] [PATCH] qemu-ns: Detect /dev/* mount point duplicates even better

[Ján Tomko]

On Wed, Nov 08, 2017 at 04:09:57PM +0100, Michal Privoznik wrote:

In 4f1570720218302 I've tried to make duplicates detection for
nested /dev mount better. However, I've missed the obvious case
when there are two same mount points. For instance if:

 # mount --bind /dev/blah /dev/blah
 # mount --bind /dev/blah /dev/blah

Yeah, very unlikely but possible.

Signed-off-by: Michal Privoznik 
---
src/qemu/qemu_domain.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)



ACK with the likeliness comment removed from the commit message

Jan


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

Re: [libvirt] [PATCH 00/11] Make auto completion better

[Martin Kletzander]

On Tue, Nov 07, 2017 at 01:22:48PM +0100, Michal Privoznik wrote:

After initial RFC [1] I had some time to work on this and here is
the result.


What's implemented?
===
Auto completion for both interactive and non-interactive
virsh/virt-admin.


Known limitations
=
Currently, just options completers work. I mean, to bring up list
of domains you have to:

 virsh # start --domain 

Doing bare:

 virsh # start 

brings up list of --options. I am working on this meanwhile. But
one can argue that having to use --option is not that much of a
problem since we have good completion now.

The other known limitation - and actually room for others to join
and help - is missing completers. I mean, the last 3 patches give
an example how to do that. But that is still very few. We need
more completers.


How does this work?
===
The basic idea is simple, when defining opts for command two new
struct members can be set:

{.name = "mac",
 .type = VSH_OT_STRING,
+ .completer = virshDomainInterfaceCompleter,
+ .completer_flags = VIRSH_DOMAIN_INTERFACE_COMPLETER_MAC,
 .help = N_("MAC address")
},

@completer is the callback that is used when user wants to bring
up list of possible strings. @completer_flags can then be used to
refine return value of @completer. In this specific example,
virshDomainInterfaceCompleter() brings up list of interfaces for
given domain. It tries to return /interface/target/@dev but if
not found (e.g. because domain is not running),
/interface/mac/@address is returned otherwise. However, in one
specific case we are interested only in MAC addresses (regardless
of domain state) and thus VIRSH_DOMAIN_INTERFACE_COMPLETER_MAC
flag is specified which alters @completer behaviour.

For non-interactive virsh/virt-admin there's
tools/bash-completion/vsh script which does no more than calling:

 $1 complete $USER_INPUT

(where $1 is name of the binary user intents to run).


How to test this?
=

Interactive completion should work out of the box. Just make sure
you're connected. Completers don't connect! You certainly don't
want ssh's 'Password:' prompt show on , do you?
Non-interactive completers do connect, but in order to avoid any
password prompts, /dev/stdin is closed before anything else is
done. In order to test it, just:

 libvirt.git $ source tools/bash-completion/vsh

Now, bash completion should work:

 libvirt.git $ ./tools/virsh -c qemu:///system start --domain 



So if I do this, it works, but if I copy it where it is supposed to be installed
it doesn't for some reason.  I haven't dug into that, I don't even know where to
start, probably.  Also it is very possible that it's my fault.

I also found one more thing, but that's only easy to fix in few cases.  If you
type 'virsh start --domain fedora ' then one of the strings it offers
is '--domain'.  It'd be nice to have that filtered out.  Not needed for ACK,
though.  Just an idea for you.

Apart from that it looks sane, so ACK to the general concept, I would just
change 2/11 as written in the reply, drop (or completely replace) 3/11 and
instead do the '-qq' thing.  And of course adjust other code depending on that.
I'd ACK the rest, I would just like to have the middle-of-the-command completion
working too so it doesn't confuse people.


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

Re: [libvirt] [PATCH 07/11] vsh: Introduce complete command

[Martin Kletzander]

On Tue, Nov 07, 2017 at 01:22:55PM +0100, Michal Privoznik wrote:

This command is going to be called from bash completion script in
the following form:

 virsh complete "start --domain"

Its only purpose is to return list of possible strings for
completion. Note that this is a 'hidden', unlisted command and
therefore there's no documentation to it.

Signed-off-by: Michal Privoznik 
---
tools/virsh.c  |  1 +
tools/virt-admin.c |  1 +
tools/vsh.c| 68 ++
tools/vsh.h| 14 +++
4 files changed, 84 insertions(+)

diff --git a/tools/virsh.c b/tools/virsh.c
index 7d6dc2620..f830331f6 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -846,6 +846,7 @@ static const vshCmdDef virshCmds[] = {
VSH_CMD_PWD,
VSH_CMD_QUIT,
VSH_CMD_SELF_TEST,
+VSH_CMD_COMPLETE,
{.name = "connect",
 .handler = cmdConnect,
 .opts = opts_connect,
diff --git a/tools/virt-admin.c b/tools/virt-admin.c
index 5d7ef7988..c24ed95c0 100644
--- a/tools/virt-admin.c
+++ b/tools/virt-admin.c
@@ -1356,6 +1356,7 @@ static const vshCmdDef vshAdmCmds[] = {
VSH_CMD_PWD,
VSH_CMD_QUIT,
VSH_CMD_SELF_TEST,
+VSH_CMD_COMPLETE,
{.name = "uri",
 .handler = cmdURI,
 .opts = NULL,
diff --git a/tools/vsh.c b/tools/vsh.c
index 121669574..136acb0ab 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -3419,3 +3419,71 @@ cmdSelfTest(vshControl *ctl ATTRIBUTE_UNUSED,

return true;
}
+
+/* --
+ * Autocompletion command
+ * -- */
+
+const vshCmdOptDef opts_complete[] = {
+{.name = "string",
+ .type = VSH_OT_ARGV,
+ .flags = VSH_OFLAG_EMPTY_OK,
+ .help = N_("partial string to autocomplete")
+},
+{.name = NULL}
+};
+
+const vshCmdInfo info_complete[] = {
+{.name = "help",
+ .data = N_("internal command for autocompletion")
+},
+{.name = "desc",
+ .data = N_("internal use only")
+},
+{.name = NULL}
+};
+
+bool
+cmdComplete(vshControl *ctl, const vshCmd *cmd)
+{
+bool ret = false;
+#ifdef WITH_READLINE
+const vshClientHooks *hooks = ctl->hooks;
+int stdin_fileno = STDIN_FILENO;
+const char *arg = NULL;
+char **matches = NULL, *tmp = NULL, **iter;
+
+if (vshCommandOptStringQuiet(ctl, cmd, "string", ) <= 0)
+goto cleanup;
+
+/* This command is flagged VSH_CMD_FLAG_NOCONNECT because we
+ * need to prevent auth hooks reading any input. Therefore we
+ * have to close stdin and then connect ourselves. */
+VIR_FORCE_CLOSE(stdin_fileno);
+
+if (!(hooks && hooks->connHandler && hooks->connHandler(ctl, true)))
+goto cleanup;
+
+autoCompleteOpaque = ctl;
+
+rl_basic_word_break_characters = " \t\n\\`@$><=;|&{(";
+if (VIR_STRDUP(rl_line_buffer, arg) < 0)
+goto cleanup;
+
+while ((tmp = strpbrk(arg, rl_basic_word_break_characters)))
+arg = tmp + 1;
+
+if (!(matches = vshReadlineCompletion(arg, 0, 0)))
+goto cleanup;
+


One more thing here, you're skipping to the last arg here, I guess this is the
reason why  doesn't work in the middle of the input, only at the end.
Maybe there should be another optional parameter that would tell you where the
completion was requested.

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

Re: [libvirt] [PATCH 07/11] vsh: Introduce complete command

[Martin Kletzander]

On Wed, Nov 08, 2017 at 04:00:35PM +0100, Michal Privoznik wrote:

On 11/08/2017 03:23 PM, Martin Kletzander wrote:

On Tue, Nov 07, 2017 at 01:22:55PM +0100, Michal Privoznik wrote:

This command is going to be called from bash completion script in
the following form:

 virsh complete "start --domain"

Its only purpose is to return list of possible strings for
completion. Note that this is a 'hidden', unlisted command and
therefore there's no documentation to it.

Signed-off-by: Michal Privoznik 
---
tools/virsh.c  |  1 +
tools/virt-admin.c |  1 +
tools/vsh.c    | 68
++
tools/vsh.h    | 14 +++
4 files changed, 84 insertions(+)

diff --git a/tools/virsh.c b/tools/virsh.c
index 7d6dc2620..f830331f6 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -846,6 +846,7 @@ static const vshCmdDef virshCmds[] = {
    VSH_CMD_PWD,
    VSH_CMD_QUIT,
    VSH_CMD_SELF_TEST,
+    VSH_CMD_COMPLETE,
    {.name = "connect",
 .handler = cmdConnect,
 .opts = opts_connect,
diff --git a/tools/virt-admin.c b/tools/virt-admin.c
index 5d7ef7988..c24ed95c0 100644
--- a/tools/virt-admin.c
+++ b/tools/virt-admin.c
@@ -1356,6 +1356,7 @@ static const vshCmdDef vshAdmCmds[] = {
    VSH_CMD_PWD,
    VSH_CMD_QUIT,
    VSH_CMD_SELF_TEST,
+    VSH_CMD_COMPLETE,
    {.name = "uri",
 .handler = cmdURI,
 .opts = NULL,
diff --git a/tools/vsh.c b/tools/vsh.c
index 121669574..136acb0ab 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -3419,3 +3419,71 @@ cmdSelfTest(vshControl *ctl ATTRIBUTE_UNUSED,

    return true;
}
+
+/* --
+ * Autocompletion command
+ * -- */
+
+const vshCmdOptDef opts_complete[] = {
+    {.name = "string",
+ .type = VSH_OT_ARGV,
+ .flags = VSH_OFLAG_EMPTY_OK,
+ .help = N_("partial string to autocomplete")
+    },
+    {.name = NULL}
+};
+
+const vshCmdInfo info_complete[] = {
+    {.name = "help",
+ .data = N_("internal command for autocompletion")
+    },
+    {.name = "desc",
+ .data = N_("internal use only")
+    },
+    {.name = NULL}
+};
+
+bool
+cmdComplete(vshControl *ctl, const vshCmd *cmd)
+{
+    bool ret = false;
+#ifdef WITH_READLINE
+    const vshClientHooks *hooks = ctl->hooks;
+    int stdin_fileno = STDIN_FILENO;
+    const char *arg = NULL;
+    char **matches = NULL, *tmp = NULL, **iter;
+
+    if (vshCommandOptStringQuiet(ctl, cmd, "string", ) <= 0)
+    goto cleanup;
+
+    /* This command is flagged VSH_CMD_FLAG_NOCONNECT because we
+ * need to prevent auth hooks reading any input. Therefore we
+ * have to close stdin and then connect ourselves. */
+    VIR_FORCE_CLOSE(stdin_fileno);
+
+    if (!(hooks && hooks->connHandler && hooks->connHandler(ctl, true)))
+    goto cleanup;
+
+    autoCompleteOpaque = ctl;
+
+    rl_basic_word_break_characters = " \t\n\\`@$><=;|&{(";
+    if (VIR_STRDUP(rl_line_buffer, arg) < 0)
+    goto cleanup;
+
+    while ((tmp = strpbrk(arg, rl_basic_word_break_characters)))
+    arg = tmp + 1;
+
+    if (!(matches = vshReadlineCompletion(arg, 0, 0)))
+    goto cleanup;
+
+    for (iter = matches; *iter; iter++)
+    printf("%s\n", *iter);
+
+    ret = true;
+ cleanup:
+    for (iter = matches; iter && *iter; iter++)
+    VIR_FREE(*iter);
+    VIR_FREE(matches);
+#endif /* WITH_READLINE */


Do we really need readline for it?  Did I miss something or are we
really just using it here just so we don't have to split the
vshReadlineParse() in some way?  Don't get me wrong, I'm OK with that,
the split would be too complicated and who doesn't use readline, right?
It just feels weird.


Yes we do need readline unfortunately. vshReadlineCompletion() calls
rl_completion_matches() which filters strings returned by
vshReadlineParse() so that list of strings presented to user corresponds
to their input. We could reimplement the rl_* function ourselves if we
really want to be readline independent. On the other hand - readline is
everywhere, so why should we bother?



Sure, I agree, my question was if this is using it just for filtering strings,
looks like yes :D


Michal

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


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

Re: [libvirt] [PATCH 08/11] tools: Provide bash autompletion file

[Michal Privoznik]

On 11/08/2017 04:18 PM, Martin Kletzander wrote:
> On Wed, Nov 08, 2017 at 04:09:10PM +0100, Michal Privoznik wrote:
>> On 11/08/2017 03:46 PM, Martin Kletzander wrote:
>>> On Tue, Nov 07, 2017 at 01:22:56PM +0100, Michal Privoznik wrote:
 Signed-off-by: Michal Privoznik 
 ---
 configure.ac   |  3 ++
 libvirt.spec.in    |  2 ++
 m4/virt-bash-completion.m4 | 74
 ++
 tools/Makefile.am  | 22 --
 tools/bash-completion/vsh  | 73
 +
 5 files changed, 172 insertions(+), 2 deletions(-)
 create mode 100644 m4/virt-bash-completion.m4
 create mode 100644 tools/bash-completion/vsh

 diff --git a/configure.ac b/configure.ac
 index b2d991c3b..9103612bb 100644
 --- a/configure.ac
 +++ b/configure.ac
 @@ -242,6 +242,7 @@ LIBVIRT_ARG_APPARMOR
 LIBVIRT_ARG_ATTR
 LIBVIRT_ARG_AUDIT
 LIBVIRT_ARG_AVAHI
 +LIBVIRT_ARG_BASH_COMPLETION
 LIBVIRT_ARG_BLKID
 LIBVIRT_ARG_CAPNG
 LIBVIRT_ARG_CURL
 @@ -278,6 +279,7 @@ LIBVIRT_CHECK_ATOMIC
 LIBVIRT_CHECK_ATTR
 LIBVIRT_CHECK_AUDIT
 LIBVIRT_CHECK_AVAHI
 +LIBVIRT_CHECK_BASH_COMPLETION
 LIBVIRT_CHECK_BLKID
 LIBVIRT_CHECK_CAPNG
 LIBVIRT_CHECK_CURL
 @@ -976,6 +978,7 @@ LIBVIRT_RESULT_APPARMOR
 LIBVIRT_RESULT_ATTR
 LIBVIRT_RESULT_AUDIT
 LIBVIRT_RESULT_AVAHI
 +LIBVIRT_RESULT_BASH_COMPLETION
 LIBVIRT_RESULT_BLKID
 LIBVIRT_RESULT_CAPNG
 LIBVIRT_RESULT_CURL
 diff --git a/libvirt.spec.in b/libvirt.spec.in
 index b00689cab..67bbd128c 100644
 --- a/libvirt.spec.in
 +++ b/libvirt.spec.in
 @@ -2043,6 +2043,8 @@ exit 0
 %{_datadir}/systemtap/tapset/libvirt_qemu_probes*.stp
 %{_datadir}/systemtap/tapset/libvirt_functions.stp

 +%{_datadir}/bash-completion/completions/vsh
 +

 %if %{with_systemd}
 %{_unitdir}/libvirt-guests.service
 diff --git a/m4/virt-bash-completion.m4 b/m4/virt-bash-completion.m4
 new file mode 100644
 index 0..e1ef58740
 --- /dev/null
 +++ b/m4/virt-bash-completion.m4
 @@ -0,0 +1,74 @@
 +dnl Bash completion support
 +dnl
 +dnl Copyright (C) 2017 Red Hat, Inc.
 +dnl
 +dnl This library is free software; you can redistribute it and/or
 +dnl modify it under the terms of the GNU Lesser General Public
 +dnl License as published by the Free Software Foundation; either
 +dnl version 2.1 of the License, or (at your option) any later version.
 +dnl
 +dnl This library is distributed in the hope that it will be useful,
 +dnl but WITHOUT ANY WARRANTY; without even the implied warranty of
 +dnl MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
 +dnl Lesser General Public License for more details.
 +dnl
 +dnl You should have received a copy of the GNU Lesser General Public
 +dnl License along with this library.  If not, see
 +dnl .
 +dnl
 +dnl Inspired by libguestfs code.
 +dnl
 +
 +AC_DEFUN([LIBVIRT_ARG_BASH_COMPLETION],[
 +  LIBVIRT_ARG_WITH_FEATURE([BASH_COMPLETION], [bash-completion],
 [check], [2.0])
 +  LIBVIRT_ARG_WITH([BASH_COMPLETIONS_DIR],
 +   [directory containing bash completions scripts],
 +   [check])
 +])
 +
 +AC_DEFUN([LIBVIRT_CHECK_BASH_COMPLETION], [
 +  AC_REQUIRE([LIBVIRT_CHECK_READLINE])
 +
 +  if test "x$with_readline" != "xyes" ; then
 +    if test "x$with_bash_completion" != "xyes" ; then
 +  with_bash_completion=no
 +    else
 +  AC_MSG_ERROR([readline is required for bash completion support])
 +    fi
 +  else
 +    if test "x$with_bash_completion" = "xcheck" ; then
 +  with_bash_completion=yes
 +    fi
>>>
>>> You should change the "check" to "yes" only after everything below
>>> succeeded.
>>>
 +  fi
 +
 +  LIBVIRT_CHECK_PKG([BASH_COMPLETION], [bash-completion], [2.0])
 +
 +  if test "x$with_bash_completion" = "xyes" ; then
 +    if test "x$with_bash_completions_dir" = "xcheck"; then
 +  AC_MSG_CHECKING([for bash-completions directory])
 +  BASH_COMPLETIONS_DIR="$($PKG_CONFIG --variable=completionsdir
 bash-completion)"
 +  AC_MSG_RESULT([$BASH_COMPLETIONS_DIR])
 +
 +  dnl Replace bash completions's exec_prefix with our own.
 +  dnl Note that ${exec_prefix} is kept verbatim at this point in
 time,
 +  dnl and will only be expanded later, when make is called: this
 makes
 +  dnl it possible to override such prefix at compilation or
 installation
 +  dnl time
 +  bash_completions_prefix="$($PKG_CONFIG --variable=prefix
 bash-completion)"
 +  if test "x$bash_completions_prefix" = "x" ; then
 +    

Re: [libvirt] [PATCH 08/11] tools: Provide bash autompletion file

[Martin Kletzander]

On Wed, Nov 08, 2017 at 04:09:10PM +0100, Michal Privoznik wrote:

On 11/08/2017 03:46 PM, Martin Kletzander wrote:

On Tue, Nov 07, 2017 at 01:22:56PM +0100, Michal Privoznik wrote:

Signed-off-by: Michal Privoznik 
---
configure.ac   |  3 ++
libvirt.spec.in    |  2 ++
m4/virt-bash-completion.m4 | 74
++
tools/Makefile.am  | 22 --
tools/bash-completion/vsh  | 73
+
5 files changed, 172 insertions(+), 2 deletions(-)
create mode 100644 m4/virt-bash-completion.m4
create mode 100644 tools/bash-completion/vsh

diff --git a/configure.ac b/configure.ac
index b2d991c3b..9103612bb 100644
--- a/configure.ac
+++ b/configure.ac
@@ -242,6 +242,7 @@ LIBVIRT_ARG_APPARMOR
LIBVIRT_ARG_ATTR
LIBVIRT_ARG_AUDIT
LIBVIRT_ARG_AVAHI
+LIBVIRT_ARG_BASH_COMPLETION
LIBVIRT_ARG_BLKID
LIBVIRT_ARG_CAPNG
LIBVIRT_ARG_CURL
@@ -278,6 +279,7 @@ LIBVIRT_CHECK_ATOMIC
LIBVIRT_CHECK_ATTR
LIBVIRT_CHECK_AUDIT
LIBVIRT_CHECK_AVAHI
+LIBVIRT_CHECK_BASH_COMPLETION
LIBVIRT_CHECK_BLKID
LIBVIRT_CHECK_CAPNG
LIBVIRT_CHECK_CURL
@@ -976,6 +978,7 @@ LIBVIRT_RESULT_APPARMOR
LIBVIRT_RESULT_ATTR
LIBVIRT_RESULT_AUDIT
LIBVIRT_RESULT_AVAHI
+LIBVIRT_RESULT_BASH_COMPLETION
LIBVIRT_RESULT_BLKID
LIBVIRT_RESULT_CAPNG
LIBVIRT_RESULT_CURL
diff --git a/libvirt.spec.in b/libvirt.spec.in
index b00689cab..67bbd128c 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -2043,6 +2043,8 @@ exit 0
%{_datadir}/systemtap/tapset/libvirt_qemu_probes*.stp
%{_datadir}/systemtap/tapset/libvirt_functions.stp

+%{_datadir}/bash-completion/completions/vsh
+

%if %{with_systemd}
%{_unitdir}/libvirt-guests.service
diff --git a/m4/virt-bash-completion.m4 b/m4/virt-bash-completion.m4
new file mode 100644
index 0..e1ef58740
--- /dev/null
+++ b/m4/virt-bash-completion.m4
@@ -0,0 +1,74 @@
+dnl Bash completion support
+dnl
+dnl Copyright (C) 2017 Red Hat, Inc.
+dnl
+dnl This library is free software; you can redistribute it and/or
+dnl modify it under the terms of the GNU Lesser General Public
+dnl License as published by the Free Software Foundation; either
+dnl version 2.1 of the License, or (at your option) any later version.
+dnl
+dnl This library is distributed in the hope that it will be useful,
+dnl but WITHOUT ANY WARRANTY; without even the implied warranty of
+dnl MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+dnl Lesser General Public License for more details.
+dnl
+dnl You should have received a copy of the GNU Lesser General Public
+dnl License along with this library.  If not, see
+dnl .
+dnl
+dnl Inspired by libguestfs code.
+dnl
+
+AC_DEFUN([LIBVIRT_ARG_BASH_COMPLETION],[
+  LIBVIRT_ARG_WITH_FEATURE([BASH_COMPLETION], [bash-completion],
[check], [2.0])
+  LIBVIRT_ARG_WITH([BASH_COMPLETIONS_DIR],
+   [directory containing bash completions scripts],
+   [check])
+])
+
+AC_DEFUN([LIBVIRT_CHECK_BASH_COMPLETION], [
+  AC_REQUIRE([LIBVIRT_CHECK_READLINE])
+
+  if test "x$with_readline" != "xyes" ; then
+    if test "x$with_bash_completion" != "xyes" ; then
+  with_bash_completion=no
+    else
+  AC_MSG_ERROR([readline is required for bash completion support])
+    fi
+  else
+    if test "x$with_bash_completion" = "xcheck" ; then
+  with_bash_completion=yes
+    fi


You should change the "check" to "yes" only after everything below
succeeded.


+  fi
+
+  LIBVIRT_CHECK_PKG([BASH_COMPLETION], [bash-completion], [2.0])
+
+  if test "x$with_bash_completion" = "xyes" ; then
+    if test "x$with_bash_completions_dir" = "xcheck"; then
+  AC_MSG_CHECKING([for bash-completions directory])
+  BASH_COMPLETIONS_DIR="$($PKG_CONFIG --variable=completionsdir
bash-completion)"
+  AC_MSG_RESULT([$BASH_COMPLETIONS_DIR])
+
+  dnl Replace bash completions's exec_prefix with our own.
+  dnl Note that ${exec_prefix} is kept verbatim at this point in
time,
+  dnl and will only be expanded later, when make is called: this
makes
+  dnl it possible to override such prefix at compilation or
installation
+  dnl time
+  bash_completions_prefix="$($PKG_CONFIG --variable=prefix
bash-completion)"
+  if test "x$bash_completions_prefix" = "x" ; then
+    bash_completions_prefix="/usr"
+  fi
+
+ 
BASH_COMPLETIONS_DIR='${exec_prefix}'"${BASH_COMPLETIONS_DIR#$bash_completions_prefix}"

+    elif test "x$with_bash_completions_dir" = "xno" || test
"x$with_bash_completions_dir" = "xyes"; then
+  AC_MSG_ERROR([bash-completions-dir must be used only with valid
path])


Otherwise you can error out here ^^ even when nobody requested anything.


+    else
+  BASH_COMPLETIONS_DIR=$with_bash_completions_dir
+    fi
+    AC_SUBST([BASH_COMPLETIONS_DIR])
+  fi
+])
+
+AC_DEFUN([LIBVIRT_RESULT_BASH_COMPLETION],[
+  LIBVIRT_RESULT_LIB([BASH_COMPLETION])
+])
diff --git a/tools/Makefile.am b/tools/Makefile.am
index 7513a73ac..34a81e69c 

Re: [libvirt] [PATCH] qemu-ns: Detect /dev/* mount point duplicates even better

[Daniel P. Berrange]

On Wed, Nov 08, 2017 at 04:09:57PM +0100, Michal Privoznik wrote:
> In 4f1570720218302 I've tried to make duplicates detection for
> nested /dev mount better. However, I've missed the obvious case
> when there are two same mount points. For instance if:
> 
>   # mount --bind /dev/blah /dev/blah
>   # mount --bind /dev/blah /dev/blah
> 
> Yeah, very unlikely but possible.

It isn't all that unlikely - libvirt itself sets up mounts just like
that for LXC containers it spawns :-)

> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_domain.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 702463547..61d28337b 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -8352,7 +8352,7 @@ qemuDomainGetPreservedMounts(virQEMUDriverConfigPtr cfg,
>  while (j < nmounts) {
>  char *c = STRSKIP(mounts[j], mounts[i]);
>  
> -if (c && *c == '/') {
> +if (c && (*c == '/' || *c == '\0')) {
>  VIR_DEBUG("Dropping path %s because of %s", mounts[j], 
> mounts[i]);
>  VIR_DELETE_ELEMENT(mounts, j, nmounts);
>  } else {
> -- 
> 2.13.6
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

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

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

[libvirt] [PATCH] qemu-ns: Detect /dev/* mount point duplicates even better

[Michal Privoznik]

In 4f1570720218302 I've tried to make duplicates detection for
nested /dev mount better. However, I've missed the obvious case
when there are two same mount points. For instance if:

  # mount --bind /dev/blah /dev/blah
  # mount --bind /dev/blah /dev/blah

Yeah, very unlikely but possible.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_domain.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 702463547..61d28337b 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -8352,7 +8352,7 @@ qemuDomainGetPreservedMounts(virQEMUDriverConfigPtr cfg,
 while (j < nmounts) {
 char *c = STRSKIP(mounts[j], mounts[i]);
 
-if (c && *c == '/') {
+if (c && (*c == '/' || *c == '\0')) {
 VIR_DEBUG("Dropping path %s because of %s", mounts[j], 
mounts[i]);
 VIR_DELETE_ELEMENT(mounts, j, nmounts);
 } else {
-- 
2.13.6

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

Re: [libvirt] [PATCH 08/11] tools: Provide bash autompletion file

[Michal Privoznik]

On 11/08/2017 03:46 PM, Martin Kletzander wrote:
> On Tue, Nov 07, 2017 at 01:22:56PM +0100, Michal Privoznik wrote:
>> Signed-off-by: Michal Privoznik 
>> ---
>> configure.ac   |  3 ++
>> libvirt.spec.in    |  2 ++
>> m4/virt-bash-completion.m4 | 74
>> ++
>> tools/Makefile.am  | 22 --
>> tools/bash-completion/vsh  | 73
>> +
>> 5 files changed, 172 insertions(+), 2 deletions(-)
>> create mode 100644 m4/virt-bash-completion.m4
>> create mode 100644 tools/bash-completion/vsh
>>
>> diff --git a/configure.ac b/configure.ac
>> index b2d991c3b..9103612bb 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -242,6 +242,7 @@ LIBVIRT_ARG_APPARMOR
>> LIBVIRT_ARG_ATTR
>> LIBVIRT_ARG_AUDIT
>> LIBVIRT_ARG_AVAHI
>> +LIBVIRT_ARG_BASH_COMPLETION
>> LIBVIRT_ARG_BLKID
>> LIBVIRT_ARG_CAPNG
>> LIBVIRT_ARG_CURL
>> @@ -278,6 +279,7 @@ LIBVIRT_CHECK_ATOMIC
>> LIBVIRT_CHECK_ATTR
>> LIBVIRT_CHECK_AUDIT
>> LIBVIRT_CHECK_AVAHI
>> +LIBVIRT_CHECK_BASH_COMPLETION
>> LIBVIRT_CHECK_BLKID
>> LIBVIRT_CHECK_CAPNG
>> LIBVIRT_CHECK_CURL
>> @@ -976,6 +978,7 @@ LIBVIRT_RESULT_APPARMOR
>> LIBVIRT_RESULT_ATTR
>> LIBVIRT_RESULT_AUDIT
>> LIBVIRT_RESULT_AVAHI
>> +LIBVIRT_RESULT_BASH_COMPLETION
>> LIBVIRT_RESULT_BLKID
>> LIBVIRT_RESULT_CAPNG
>> LIBVIRT_RESULT_CURL
>> diff --git a/libvirt.spec.in b/libvirt.spec.in
>> index b00689cab..67bbd128c 100644
>> --- a/libvirt.spec.in
>> +++ b/libvirt.spec.in
>> @@ -2043,6 +2043,8 @@ exit 0
>> %{_datadir}/systemtap/tapset/libvirt_qemu_probes*.stp
>> %{_datadir}/systemtap/tapset/libvirt_functions.stp
>>
>> +%{_datadir}/bash-completion/completions/vsh
>> +
>>
>> %if %{with_systemd}
>> %{_unitdir}/libvirt-guests.service
>> diff --git a/m4/virt-bash-completion.m4 b/m4/virt-bash-completion.m4
>> new file mode 100644
>> index 0..e1ef58740
>> --- /dev/null
>> +++ b/m4/virt-bash-completion.m4
>> @@ -0,0 +1,74 @@
>> +dnl Bash completion support
>> +dnl
>> +dnl Copyright (C) 2017 Red Hat, Inc.
>> +dnl
>> +dnl This library is free software; you can redistribute it and/or
>> +dnl modify it under the terms of the GNU Lesser General Public
>> +dnl License as published by the Free Software Foundation; either
>> +dnl version 2.1 of the License, or (at your option) any later version.
>> +dnl
>> +dnl This library is distributed in the hope that it will be useful,
>> +dnl but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +dnl MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> +dnl Lesser General Public License for more details.
>> +dnl
>> +dnl You should have received a copy of the GNU Lesser General Public
>> +dnl License along with this library.  If not, see
>> +dnl .
>> +dnl
>> +dnl Inspired by libguestfs code.
>> +dnl
>> +
>> +AC_DEFUN([LIBVIRT_ARG_BASH_COMPLETION],[
>> +  LIBVIRT_ARG_WITH_FEATURE([BASH_COMPLETION], [bash-completion],
>> [check], [2.0])
>> +  LIBVIRT_ARG_WITH([BASH_COMPLETIONS_DIR],
>> +   [directory containing bash completions scripts],
>> +   [check])
>> +])
>> +
>> +AC_DEFUN([LIBVIRT_CHECK_BASH_COMPLETION], [
>> +  AC_REQUIRE([LIBVIRT_CHECK_READLINE])
>> +
>> +  if test "x$with_readline" != "xyes" ; then
>> +    if test "x$with_bash_completion" != "xyes" ; then
>> +  with_bash_completion=no
>> +    else
>> +  AC_MSG_ERROR([readline is required for bash completion support])
>> +    fi
>> +  else
>> +    if test "x$with_bash_completion" = "xcheck" ; then
>> +  with_bash_completion=yes
>> +    fi
> 
> You should change the "check" to "yes" only after everything below
> succeeded.
> 
>> +  fi
>> +
>> +  LIBVIRT_CHECK_PKG([BASH_COMPLETION], [bash-completion], [2.0])
>> +
>> +  if test "x$with_bash_completion" = "xyes" ; then
>> +    if test "x$with_bash_completions_dir" = "xcheck"; then
>> +  AC_MSG_CHECKING([for bash-completions directory])
>> +  BASH_COMPLETIONS_DIR="$($PKG_CONFIG --variable=completionsdir
>> bash-completion)"
>> +  AC_MSG_RESULT([$BASH_COMPLETIONS_DIR])
>> +
>> +  dnl Replace bash completions's exec_prefix with our own.
>> +  dnl Note that ${exec_prefix} is kept verbatim at this point in
>> time,
>> +  dnl and will only be expanded later, when make is called: this
>> makes
>> +  dnl it possible to override such prefix at compilation or
>> installation
>> +  dnl time
>> +  bash_completions_prefix="$($PKG_CONFIG --variable=prefix
>> bash-completion)"
>> +  if test "x$bash_completions_prefix" = "x" ; then
>> +    bash_completions_prefix="/usr"
>> +  fi
>> +
>> + 
>> BASH_COMPLETIONS_DIR='${exec_prefix}'"${BASH_COMPLETIONS_DIR#$bash_completions_prefix}"
>>
>> +    elif test "x$with_bash_completions_dir" = "xno" || test
>> "x$with_bash_completions_dir" = "xyes"; then
>> +  AC_MSG_ERROR([bash-completions-dir must be used only with valid
>> path])
> 
> Otherwise you can error 

Re: [libvirt] [PATCH 07/11] vsh: Introduce complete command

[Michal Privoznik]

On 11/08/2017 03:23 PM, Martin Kletzander wrote:
> On Tue, Nov 07, 2017 at 01:22:55PM +0100, Michal Privoznik wrote:
>> This command is going to be called from bash completion script in
>> the following form:
>>
>>  virsh complete "start --domain"
>>
>> Its only purpose is to return list of possible strings for
>> completion. Note that this is a 'hidden', unlisted command and
>> therefore there's no documentation to it.
>>
>> Signed-off-by: Michal Privoznik 
>> ---
>> tools/virsh.c  |  1 +
>> tools/virt-admin.c |  1 +
>> tools/vsh.c    | 68
>> ++
>> tools/vsh.h    | 14 +++
>> 4 files changed, 84 insertions(+)
>>
>> diff --git a/tools/virsh.c b/tools/virsh.c
>> index 7d6dc2620..f830331f6 100644
>> --- a/tools/virsh.c
>> +++ b/tools/virsh.c
>> @@ -846,6 +846,7 @@ static const vshCmdDef virshCmds[] = {
>>     VSH_CMD_PWD,
>>     VSH_CMD_QUIT,
>>     VSH_CMD_SELF_TEST,
>> +    VSH_CMD_COMPLETE,
>>     {.name = "connect",
>>  .handler = cmdConnect,
>>  .opts = opts_connect,
>> diff --git a/tools/virt-admin.c b/tools/virt-admin.c
>> index 5d7ef7988..c24ed95c0 100644
>> --- a/tools/virt-admin.c
>> +++ b/tools/virt-admin.c
>> @@ -1356,6 +1356,7 @@ static const vshCmdDef vshAdmCmds[] = {
>>     VSH_CMD_PWD,
>>     VSH_CMD_QUIT,
>>     VSH_CMD_SELF_TEST,
>> +    VSH_CMD_COMPLETE,
>>     {.name = "uri",
>>  .handler = cmdURI,
>>  .opts = NULL,
>> diff --git a/tools/vsh.c b/tools/vsh.c
>> index 121669574..136acb0ab 100644
>> --- a/tools/vsh.c
>> +++ b/tools/vsh.c
>> @@ -3419,3 +3419,71 @@ cmdSelfTest(vshControl *ctl ATTRIBUTE_UNUSED,
>>
>>     return true;
>> }
>> +
>> +/* --
>> + * Autocompletion command
>> + * -- */
>> +
>> +const vshCmdOptDef opts_complete[] = {
>> +    {.name = "string",
>> + .type = VSH_OT_ARGV,
>> + .flags = VSH_OFLAG_EMPTY_OK,
>> + .help = N_("partial string to autocomplete")
>> +    },
>> +    {.name = NULL}
>> +};
>> +
>> +const vshCmdInfo info_complete[] = {
>> +    {.name = "help",
>> + .data = N_("internal command for autocompletion")
>> +    },
>> +    {.name = "desc",
>> + .data = N_("internal use only")
>> +    },
>> +    {.name = NULL}
>> +};
>> +
>> +bool
>> +cmdComplete(vshControl *ctl, const vshCmd *cmd)
>> +{
>> +    bool ret = false;
>> +#ifdef WITH_READLINE
>> +    const vshClientHooks *hooks = ctl->hooks;
>> +    int stdin_fileno = STDIN_FILENO;
>> +    const char *arg = NULL;
>> +    char **matches = NULL, *tmp = NULL, **iter;
>> +
>> +    if (vshCommandOptStringQuiet(ctl, cmd, "string", ) <= 0)
>> +    goto cleanup;
>> +
>> +    /* This command is flagged VSH_CMD_FLAG_NOCONNECT because we
>> + * need to prevent auth hooks reading any input. Therefore we
>> + * have to close stdin and then connect ourselves. */
>> +    VIR_FORCE_CLOSE(stdin_fileno);
>> +
>> +    if (!(hooks && hooks->connHandler && hooks->connHandler(ctl, true)))
>> +    goto cleanup;
>> +
>> +    autoCompleteOpaque = ctl;
>> +
>> +    rl_basic_word_break_characters = " \t\n\\`@$><=;|&{(";
>> +    if (VIR_STRDUP(rl_line_buffer, arg) < 0)
>> +    goto cleanup;
>> +
>> +    while ((tmp = strpbrk(arg, rl_basic_word_break_characters)))
>> +    arg = tmp + 1;
>> +
>> +    if (!(matches = vshReadlineCompletion(arg, 0, 0)))
>> +    goto cleanup;
>> +
>> +    for (iter = matches; *iter; iter++)
>> +    printf("%s\n", *iter);
>> +
>> +    ret = true;
>> + cleanup:
>> +    for (iter = matches; iter && *iter; iter++)
>> +    VIR_FREE(*iter);
>> +    VIR_FREE(matches);
>> +#endif /* WITH_READLINE */
> 
> Do we really need readline for it?  Did I miss something or are we
> really just using it here just so we don't have to split the
> vshReadlineParse() in some way?  Don't get me wrong, I'm OK with that,
> the split would be too complicated and who doesn't use readline, right?
> It just feels weird.

Yes we do need readline unfortunately. vshReadlineCompletion() calls
rl_completion_matches() which filters strings returned by
vshReadlineParse() so that list of strings presented to user corresponds
to their input. We could reimplement the rl_* function ourselves if we
really want to be readline independent. On the other hand - readline is
everywhere, so why should we bother?

Michal

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

[libvirt] [PATCH] conf: Properly parse

[Peter Krempa]

The terminator would not be parsed properly since the XPath selector was
looking for an populated element, and also the code did not bother
assigning the terminating virStorageSourcePtr to the backingStore
property of the parent.

Some tests would catch it if there wasn't bigger fallout from the change
to backing store termination in a693fdba0111. Fix them properly now.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1509110
---
 src/conf/domain_conf.c | 14 ++
 .../qemuxml2xmlout-disk-active-commit.xml  |  1 +
 .../qemuxml2xmlout-disk-backing-chains-active.xml  |  3 +++
 .../qemuxml2xmlout-disk-mirror-active.xml  |  4 
 .../qemuxml2xmloutdata/qemuxml2xmlout-disk-mirror-old.xml  |  4 
 .../qemuxml2xmlout-seclabel-static-labelskip.xml   |  1 +
 6 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 7dfd7b54e..a1ca307c4 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -8545,23 +8545,21 @@ virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt,
 char *idx = NULL;
 int ret = -1;

-if (!(ctxt->node = virXPathNode("./backingStore[*]", ctxt))) {
+if (!(ctxt->node = virXPathNode("./backingStore", ctxt))) {
 ret = 0;
 goto cleanup;
 }

-if (!(type = virXMLPropString(ctxt->node, "type"))) {
-/* terminator does not have a type */
-if (VIR_ALLOC(backingStore) < 0)
-goto cleanup;
+if (VIR_ALLOC(backingStore) < 0)
+goto cleanup;

+/* terminator does not have a type */
+if (!(type = virXMLPropString(ctxt->node, "type"))) {
+VIR_STEAL_PTR(src->backingStore, backingStore);
 ret = 0;
 goto cleanup;
 }

-if (VIR_ALLOC(backingStore) < 0)
-goto cleanup;
-
 if (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) &&
 (idx = virXMLPropString(ctxt->node, "index")) &&
 virStrToLong_uip(idx, NULL, 10, >id) < 0) {
diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-active-commit.xml 
b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-active-commit.xml
index 5766e4aea..cc26af109 100644
--- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-active-commit.xml
+++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-active-commit.xml
@@ -20,6 +20,7 @@
   
 
 
+
   
   
 
diff --git 
a/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-backing-chains-active.xml 
b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-backing-chains-active.xml
index 828defcc2..d1fd2442c 100644
--- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-backing-chains-active.xml
+++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-backing-chains-active.xml
@@ -49,6 +49,7 @@
 
   
   
+  
 
   
 
@@ -63,6 +64,7 @@
   
 
   
+  
   
   
 
@@ -79,6 +81,7 @@
   
 
 
+
   
   
   
diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-mirror-active.xml 
b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-mirror-active.xml
index 252bde338..c1e8a33ec 100644
--- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-mirror-active.xml
+++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-mirror-active.xml
@@ -16,6 +16,7 @@
 /usr/bin/qemu-system-i686
 
   
+  
   
 
   
@@ -24,12 +25,14 @@
 
 
   
+  
   
   
   
 
 
   
+  
   
 
 
@@ -39,6 +42,7 @@
 
 
   
+  
   
 
 
diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-mirror-old.xml 
b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-mirror-old.xml
index f4bd39a58..e390bc02f 100644
--- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-mirror-old.xml
+++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-mirror-old.xml
@@ -16,6 +16,7 @@
 /usr/bin/qemu-system-i686
 
   
+  
   
 
   
@@ -24,12 +25,14 @@
 
 
   
+  
   
   
   
 
 
   
+  
   
 
 
@@ -39,6 +42,7 @@
 
 
   
+  
   
   
 
diff --git 
a/tests/qemuxml2xmloutdata/qemuxml2xmlout-seclabel-static-labelskip.xml 
b/tests/qemuxml2xmloutdata/qemuxml2xmlout-seclabel-static-labelskip.xml
index 91f573db7..d37b950cb 100644
--- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-seclabel-static-labelskip.xml
+++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-seclabel-static-labelskip.xml
@@ -18,6 +18,7 @@
   
 
   
+  
   
   
 
-- 
2.14.3

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

Re: [libvirt] [PATCH 08/11] tools: Provide bash autompletion file

[Martin Kletzander]

On Tue, Nov 07, 2017 at 01:22:56PM +0100, Michal Privoznik wrote:

Signed-off-by: Michal Privoznik 
---
configure.ac   |  3 ++
libvirt.spec.in|  2 ++
m4/virt-bash-completion.m4 | 74 ++
tools/Makefile.am  | 22 --
tools/bash-completion/vsh  | 73 +
5 files changed, 172 insertions(+), 2 deletions(-)
create mode 100644 m4/virt-bash-completion.m4
create mode 100644 tools/bash-completion/vsh

diff --git a/configure.ac b/configure.ac
index b2d991c3b..9103612bb 100644
--- a/configure.ac
+++ b/configure.ac
@@ -242,6 +242,7 @@ LIBVIRT_ARG_APPARMOR
LIBVIRT_ARG_ATTR
LIBVIRT_ARG_AUDIT
LIBVIRT_ARG_AVAHI
+LIBVIRT_ARG_BASH_COMPLETION
LIBVIRT_ARG_BLKID
LIBVIRT_ARG_CAPNG
LIBVIRT_ARG_CURL
@@ -278,6 +279,7 @@ LIBVIRT_CHECK_ATOMIC
LIBVIRT_CHECK_ATTR
LIBVIRT_CHECK_AUDIT
LIBVIRT_CHECK_AVAHI
+LIBVIRT_CHECK_BASH_COMPLETION
LIBVIRT_CHECK_BLKID
LIBVIRT_CHECK_CAPNG
LIBVIRT_CHECK_CURL
@@ -976,6 +978,7 @@ LIBVIRT_RESULT_APPARMOR
LIBVIRT_RESULT_ATTR
LIBVIRT_RESULT_AUDIT
LIBVIRT_RESULT_AVAHI
+LIBVIRT_RESULT_BASH_COMPLETION
LIBVIRT_RESULT_BLKID
LIBVIRT_RESULT_CAPNG
LIBVIRT_RESULT_CURL
diff --git a/libvirt.spec.in b/libvirt.spec.in
index b00689cab..67bbd128c 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -2043,6 +2043,8 @@ exit 0
%{_datadir}/systemtap/tapset/libvirt_qemu_probes*.stp
%{_datadir}/systemtap/tapset/libvirt_functions.stp

+%{_datadir}/bash-completion/completions/vsh
+

%if %{with_systemd}
%{_unitdir}/libvirt-guests.service
diff --git a/m4/virt-bash-completion.m4 b/m4/virt-bash-completion.m4
new file mode 100644
index 0..e1ef58740
--- /dev/null
+++ b/m4/virt-bash-completion.m4
@@ -0,0 +1,74 @@
+dnl Bash completion support
+dnl
+dnl Copyright (C) 2017 Red Hat, Inc.
+dnl
+dnl This library is free software; you can redistribute it and/or
+dnl modify it under the terms of the GNU Lesser General Public
+dnl License as published by the Free Software Foundation; either
+dnl version 2.1 of the License, or (at your option) any later version.
+dnl
+dnl This library is distributed in the hope that it will be useful,
+dnl but WITHOUT ANY WARRANTY; without even the implied warranty of
+dnl MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+dnl Lesser General Public License for more details.
+dnl
+dnl You should have received a copy of the GNU Lesser General Public
+dnl License along with this library.  If not, see
+dnl .
+dnl
+dnl Inspired by libguestfs code.
+dnl
+
+AC_DEFUN([LIBVIRT_ARG_BASH_COMPLETION],[
+  LIBVIRT_ARG_WITH_FEATURE([BASH_COMPLETION], [bash-completion], [check], 
[2.0])
+  LIBVIRT_ARG_WITH([BASH_COMPLETIONS_DIR],
+   [directory containing bash completions scripts],
+   [check])
+])
+
+AC_DEFUN([LIBVIRT_CHECK_BASH_COMPLETION], [
+  AC_REQUIRE([LIBVIRT_CHECK_READLINE])
+
+  if test "x$with_readline" != "xyes" ; then
+if test "x$with_bash_completion" != "xyes" ; then
+  with_bash_completion=no
+else
+  AC_MSG_ERROR([readline is required for bash completion support])
+fi
+  else
+if test "x$with_bash_completion" = "xcheck" ; then
+  with_bash_completion=yes
+fi


You should change the "check" to "yes" only after everything below succeeded.


+  fi
+
+  LIBVIRT_CHECK_PKG([BASH_COMPLETION], [bash-completion], [2.0])
+
+  if test "x$with_bash_completion" = "xyes" ; then
+if test "x$with_bash_completions_dir" = "xcheck"; then
+  AC_MSG_CHECKING([for bash-completions directory])
+  BASH_COMPLETIONS_DIR="$($PKG_CONFIG --variable=completionsdir 
bash-completion)"
+  AC_MSG_RESULT([$BASH_COMPLETIONS_DIR])
+
+  dnl Replace bash completions's exec_prefix with our own.
+  dnl Note that ${exec_prefix} is kept verbatim at this point in time,
+  dnl and will only be expanded later, when make is called: this makes
+  dnl it possible to override such prefix at compilation or installation
+  dnl time
+  bash_completions_prefix="$($PKG_CONFIG --variable=prefix 
bash-completion)"
+  if test "x$bash_completions_prefix" = "x" ; then
+bash_completions_prefix="/usr"
+  fi
+
+  
BASH_COMPLETIONS_DIR='${exec_prefix}'"${BASH_COMPLETIONS_DIR#$bash_completions_prefix}"
+elif test "x$with_bash_completions_dir" = "xno" || test 
"x$with_bash_completions_dir" = "xyes"; then
+  AC_MSG_ERROR([bash-completions-dir must be used only with valid path])


Otherwise you can error out here ^^ even when nobody requested anything.


+else
+  BASH_COMPLETIONS_DIR=$with_bash_completions_dir
+fi
+AC_SUBST([BASH_COMPLETIONS_DIR])
+  fi
+])
+
+AC_DEFUN([LIBVIRT_RESULT_BASH_COMPLETION],[
+  LIBVIRT_RESULT_LIB([BASH_COMPLETION])
+])
diff --git a/tools/Makefile.am b/tools/Makefile.am
index 7513a73ac..34a81e69c 100644
--- a/tools/Makefile.am
+++ b/tools/Makefile.am
@@ -66,6 +66,7 @@ EXTRA_DIST = \

Re: [libvirt] [PATCH] conf: Fix type for @liveStatus in virDomainObjListLoadAllConfigs

[Jiri Denemark]

On Wed, Nov 08, 2017 at 15:18:37 +0100, Peter Krempa wrote:
> Use bool instead of an int.
> ---
>  src/bhyve/bhyve_driver.c| 4 ++--
>  src/conf/virdomainobjlist.c | 2 +-
>  src/conf/virdomainobjlist.h | 2 +-
>  src/libxl/libxl_driver.c| 6 +++---
>  src/lxc/lxc_driver.c| 6 +++---
>  src/qemu/qemu_driver.c  | 6 +++---
>  src/uml/uml_driver.c| 4 ++--
>  7 files changed, 15 insertions(+), 15 deletions(-)

Reviewed-by: Jiri Denemark 

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

Re: [libvirt] [PATCH 07/11] vsh: Introduce complete command

[Martin Kletzander]

On Tue, Nov 07, 2017 at 01:22:55PM +0100, Michal Privoznik wrote:

This command is going to be called from bash completion script in
the following form:

 virsh complete "start --domain"

Its only purpose is to return list of possible strings for
completion. Note that this is a 'hidden', unlisted command and
therefore there's no documentation to it.

Signed-off-by: Michal Privoznik 
---
tools/virsh.c  |  1 +
tools/virt-admin.c |  1 +
tools/vsh.c| 68 ++
tools/vsh.h| 14 +++
4 files changed, 84 insertions(+)

diff --git a/tools/virsh.c b/tools/virsh.c
index 7d6dc2620..f830331f6 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -846,6 +846,7 @@ static const vshCmdDef virshCmds[] = {
VSH_CMD_PWD,
VSH_CMD_QUIT,
VSH_CMD_SELF_TEST,
+VSH_CMD_COMPLETE,
{.name = "connect",
 .handler = cmdConnect,
 .opts = opts_connect,
diff --git a/tools/virt-admin.c b/tools/virt-admin.c
index 5d7ef7988..c24ed95c0 100644
--- a/tools/virt-admin.c
+++ b/tools/virt-admin.c
@@ -1356,6 +1356,7 @@ static const vshCmdDef vshAdmCmds[] = {
VSH_CMD_PWD,
VSH_CMD_QUIT,
VSH_CMD_SELF_TEST,
+VSH_CMD_COMPLETE,
{.name = "uri",
 .handler = cmdURI,
 .opts = NULL,
diff --git a/tools/vsh.c b/tools/vsh.c
index 121669574..136acb0ab 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -3419,3 +3419,71 @@ cmdSelfTest(vshControl *ctl ATTRIBUTE_UNUSED,

return true;
}
+
+/* --
+ * Autocompletion command
+ * -- */
+
+const vshCmdOptDef opts_complete[] = {
+{.name = "string",
+ .type = VSH_OT_ARGV,
+ .flags = VSH_OFLAG_EMPTY_OK,
+ .help = N_("partial string to autocomplete")
+},
+{.name = NULL}
+};
+
+const vshCmdInfo info_complete[] = {
+{.name = "help",
+ .data = N_("internal command for autocompletion")
+},
+{.name = "desc",
+ .data = N_("internal use only")
+},
+{.name = NULL}
+};
+
+bool
+cmdComplete(vshControl *ctl, const vshCmd *cmd)
+{
+bool ret = false;
+#ifdef WITH_READLINE
+const vshClientHooks *hooks = ctl->hooks;
+int stdin_fileno = STDIN_FILENO;
+const char *arg = NULL;
+char **matches = NULL, *tmp = NULL, **iter;
+
+if (vshCommandOptStringQuiet(ctl, cmd, "string", ) <= 0)
+goto cleanup;
+
+/* This command is flagged VSH_CMD_FLAG_NOCONNECT because we
+ * need to prevent auth hooks reading any input. Therefore we
+ * have to close stdin and then connect ourselves. */
+VIR_FORCE_CLOSE(stdin_fileno);
+
+if (!(hooks && hooks->connHandler && hooks->connHandler(ctl, true)))
+goto cleanup;
+
+autoCompleteOpaque = ctl;
+
+rl_basic_word_break_characters = " \t\n\\`@$><=;|&{(";
+if (VIR_STRDUP(rl_line_buffer, arg) < 0)
+goto cleanup;
+
+while ((tmp = strpbrk(arg, rl_basic_word_break_characters)))
+arg = tmp + 1;
+
+if (!(matches = vshReadlineCompletion(arg, 0, 0)))
+goto cleanup;
+
+for (iter = matches; *iter; iter++)
+printf("%s\n", *iter);
+
+ret = true;
+ cleanup:
+for (iter = matches; iter && *iter; iter++)
+VIR_FREE(*iter);
+VIR_FREE(matches);
+#endif /* WITH_READLINE */


Do we really need readline for it?  Did I miss something or are we
really just using it here just so we don't have to split the
vshReadlineParse() in some way?  Don't get me wrong, I'm OK with that,
the split would be too complicated and who doesn't use readline, right?
It just feels weird.


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

[libvirt] [PATCH] conf: Fix type for @liveStatus in virDomainObjListLoadAllConfigs

[Peter Krempa]

Use bool instead of an int.
---
 src/bhyve/bhyve_driver.c| 4 ++--
 src/conf/virdomainobjlist.c | 2 +-
 src/conf/virdomainobjlist.h | 2 +-
 src/libxl/libxl_driver.c| 6 +++---
 src/lxc/lxc_driver.c| 6 +++---
 src/qemu/qemu_driver.c  | 6 +++---
 src/uml/uml_driver.c| 4 ++--
 7 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c
index 5c432b25e..dd6e8abc6 100644
--- a/src/bhyve/bhyve_driver.c
+++ b/src/bhyve/bhyve_driver.c
@@ -1294,7 +1294,7 @@ bhyveStateInitialize(bool privileged,

 if (virDomainObjListLoadAllConfigs(bhyve_driver->domains,
BHYVE_STATE_DIR,
-   NULL, 1,
+   NULL, true,
bhyve_driver->caps,
bhyve_driver->xmlopt,
NULL, NULL) < 0)
@@ -1302,7 +1302,7 @@ bhyveStateInitialize(bool privileged,

 if (virDomainObjListLoadAllConfigs(bhyve_driver->domains,
BHYVE_CONFIG_DIR,
-   BHYVE_AUTOSTART_DIR, 0,
+   BHYVE_AUTOSTART_DIR, false,
bhyve_driver->caps,
bhyve_driver->xmlopt,
NULL, NULL) < 0)
diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c
index b9f78c572..87a742b1e 100644
--- a/src/conf/virdomainobjlist.c
+++ b/src/conf/virdomainobjlist.c
@@ -559,7 +559,7 @@ int
 virDomainObjListLoadAllConfigs(virDomainObjListPtr doms,
const char *configDir,
const char *autostartDir,
-   int liveStatus,
+   bool liveStatus,
virCapsPtr caps,
virDomainXMLOptionPtr xmlopt,
virDomainLoadConfigNotify notify,
diff --git a/src/conf/virdomainobjlist.h b/src/conf/virdomainobjlist.h
index 60220ca0d..bb186bde3 100644
--- a/src/conf/virdomainobjlist.h
+++ b/src/conf/virdomainobjlist.h
@@ -72,7 +72,7 @@ void virDomainObjListRemoveLocked(virDomainObjListPtr doms,
 int virDomainObjListLoadAllConfigs(virDomainObjListPtr doms,
const char *configDir,
const char *autostartDir,
-   int liveStatus,
+   bool liveStatus,
virCapsPtr caps,
virDomainXMLOptionPtr xmlopt,
virDomainLoadConfigNotify notify,
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 951937f14..40328a6cb 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -763,7 +763,7 @@ libxlStateInitialize(bool privileged,
 if (virDomainObjListLoadAllConfigs(libxl_driver->domains,
cfg->stateDir,
cfg->autostartDir,
-   1,
+   true,
cfg->caps,
libxl_driver->xmlopt,
NULL, NULL) < 0)
@@ -775,7 +775,7 @@ libxlStateInitialize(bool privileged,
 if (virDomainObjListLoadAllConfigs(libxl_driver->domains,
cfg->configDir,
cfg->autostartDir,
-   0,
+   false,
cfg->caps,
libxl_driver->xmlopt,
NULL, NULL) < 0)
@@ -815,7 +815,7 @@ libxlStateReload(void)
 virDomainObjListLoadAllConfigs(libxl_driver->domains,
cfg->configDir,
cfg->autostartDir,
-   1,
+   true,
cfg->caps,
libxl_driver->xmlopt,
NULL, libxl_driver);
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 0069e5e92..b3447100f 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -1679,7 +1679,7 @@ static int lxcStateInitialize(bool privileged,
 /* Get all the running persistent or transient configs first */
 if (virDomainObjListLoadAllConfigs(lxc_driver->domains,
cfg->stateDir,
-   NULL, 1,
+   NULL, true,
caps,

Re: [libvirt] [[RFC] 0/8] Implement async QEMU event handling in libvirtd.

[Daniel P. Berrange]

On Mon, Nov 06, 2017 at 06:43:12AM +0100, Prerna wrote:
> Thanks for your reply Daniel. I am still on vacation all of this week so
> have not been able to respond.
> Few questions inline:
> 
> On Thu, Oct 26, 2017 at 2:43 PM, Daniel P. Berrange 
> wrote:
> 
> > On Tue, Oct 24, 2017 at 10:34:53AM -0700, Prerna Saxena wrote:
> > >
> > > As noted in
> > > https://www.redhat.com/archives/libvir-list/2017-May/msg00016.html
> > > libvirt-QEMU driver handles all async events from the main loop.
> > > Each event handling needs the per-VM lock to make forward progress. In
> > > the case where an async event is received for the same VM which has an
> > > RPC running, the main loop is held up contending for the same lock.
> > >
> > > This impacts scalability, and should be addressed on priority.
> > >
> > > Note that libvirt does have a 2-step deferred handling for a few event
> > > categories, but (1) That is insufficient since blockign happens before
> > > the handler could disambiguate which one needs to be posted to this
> > > other queue.
> > > (2) There needs to be homogeniety.
> > >
> > > The current series builds a framework for recording and handling VM
> > > events.
> > > It initializes per-VM event queue, and a global event queue pointing to
> > > events from all the VMs. Event handling is staggered in 2 stages:
> > > - When an event is received, it is enqueued in the per-VM queue as well
> > >   as the global queues.
> > > - The global queue is built into the QEMU Driver as a threadpool
> > >   (currently with a single thread).
> > > - Enqueuing of a new event triggers the global event worker thread, which
> > >   then attempts to take a lock for this event's VM.
> > > - If the lock is available, the event worker runs the function
> > handling
> > >   this event type. Once done, it dequeues this event from the global
> > >   as well as per-VM queues.
> > > - If the lock is unavailable(ie taken by RPC thread), the event
> > worker
> > >   thread leaves this as-is and picks up the next event.
> > > - Once the RPC thread completes, it looks for events pertaining to the
> > >   VM in the per-VM event queue. It then processes the events serially
> > >   (holding the VM lock) until there are no more events remaining for
> > >   this VM. At this point, the per-VM lock is relinquished.
> >
> > One of the nice aspects of processing the QEMU events in the main event
> > loop is that handling of them is self-throttling. ie if one QEMU process
> > goes mental and issues lots of events, we'll spend alot of time processing
> > them all, but our memory usage is still bounded.
> >
> > If we take events from the VM and put them on a queue that is processed
> > asynchronously, and the event processing thread gets stuck for some
> > reason, then libvirt will end up queuing an unbounded number of events.
> > This could cause considerable memory usage in libvirt. This could be
> > exploited by a malicious VM to harm libvirt. eg a malicious QEMU could
> > stop responding to monitor RPC calls, which would tie up the RPC threads
> > and handling of RPC calls. This would in turn prevent events being
> > processed due to being unable to acquire the state lock.  Now the VM
> > can flood libvirtd with events which will all be read off the wire
> > and queued in memory, potentially forever. So libvirt memory usage
> > will balloon to an arbitrary level.
> >
> > Now, the current code isn't great when faced with malicious QEMU
> > because with the same approach, QEMU can cause libvirtd main event
> > loop to stall as you found. This feels less bad than unbounded
> > memory usage though - if libvirt uses lots of memory, this will
> > push other apps on the host into swap, and/or trigger the OOM
> > killer.
> >
> > So I agree that we need to make event processing asynchronous from
> > the main loop. When doing that through, I think we need to put an
> > upper limit on the number of events we're willing to queue from a
> > VM. When we get that limit, we should update the monitor event loop
> > watch so that we stop reading further events, until existing events
> > have been processed.
>
> We can update the watch at the monitor socket level. Ie, if we have hit
> threshold limits reading events off the monitor socket, we ignore this
> socket FD going forward. Now, this also means that we will miss any replies
> coming off the same socket. It will mean that legitimate RPC replies coming
> off the same socket will get ignored too. And in this case, we deadlock,
> since event notifications will not be processed until the ongoing RPC
> completes.

An RPC waiting for a reply should release the mutex, but have the job change
lock. So the risk would be if processing an event required obtaining the
job change lock.

IIUC, the current code should already suffer from that risk though, because
events processed directly in the main loop thread would also need to acquire
the job change lock.

One approach would 

[libvirt] [REPOST PATCH v6 6/8] qemu: Use secret objects to pass iSCSI passwords

[John Ferlan]

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

The blockdev-add code provides a mechanism to sanely provide user
and password-secret arguments for iscsi without placing them on the
command line to be viewable by a 'ps -ef' type command or needing
to create separate -iscsi devices for each disk/volume found.

So modify the iSCSI command line building to check for the presence
of the capability in order properly setup and use the domain master
secret object to encrypt the password in a secret object and alter
the parameters for the command line to utilize.

Modify the xml2argvtest to exhibit the syntax for both disk and
hostdev configurations.

Signed-off-by: John Ferlan 
---
 src/qemu/qemu_command.c| 65 +-
 src/qemu/qemu_command.h|  3 +-
 src/qemu/qemu_domain.c |  4 ++
 src/qemu/qemu_hotplug.c| 50 -
 ...xml2argv-disk-drive-network-iscsi-auth-AES.args | 41 ++
 ...uxml2argv-disk-drive-network-iscsi-auth-AES.xml | 43 ++
 ...ml2argv-hostdev-scsi-virtio-iscsi-auth-AES.args | 45 +++
 ...xml2argv-hostdev-scsi-virtio-iscsi-auth-AES.xml | 48 
 tests/qemuxml2argvtest.c   | 10 
 9 files changed, 292 insertions(+), 17 deletions(-)
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-AES.args
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-AES.xml
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth-AES.args
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth-AES.xml

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 577c76b44b..f0724223f2 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1451,7 +1451,8 @@ qemuDiskBusNeedsDeviceArg(int bus)
  * the legacy representation.
  */
 static bool
-qemuDiskSourceNeedsProps(virStorageSourcePtr src)
+qemuDiskSourceNeedsProps(virStorageSourcePtr src,
+ virQEMUCapsPtr qemuCaps)
 {
 int actualType = virStorageSourceGetActualType(src);
 
@@ -1464,6 +1465,11 @@ qemuDiskSourceNeedsProps(virStorageSourcePtr src)
 src->protocol == VIR_STORAGE_NET_PROTOCOL_VXHS)
 return true;
 
+if (actualType == VIR_STORAGE_TYPE_NETWORK &&
+src->protocol == VIR_STORAGE_NET_PROTOCOL_ISCSI &&
+virQEMUCapsGet(qemuCaps, QEMU_CAPS_ISCSI_PASSWORD_SECRET))
+return true;
+
 return false;
 }
 
@@ -1507,7 +1513,7 @@ qemuBuildDriveSourceStr(virDomainDiskDefPtr disk,
 char *source = NULL;
 int ret = -1;
 
-if (qemuDiskSourceNeedsProps(disk->src) &&
+if (qemuDiskSourceNeedsProps(disk->src, qemuCaps) &&
 !(srcprops = qemuDiskSourceGetProps(disk->src)))
 goto cleanup;
 
@@ -1573,7 +1579,9 @@ qemuBuildDriveSourceStr(virDomainDiskDefPtr disk,
 virBufferAsprintf(buf, "file.debug=%d,", cfg->glusterDebugLevel);
 }
 
-if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) {
+if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES &&
+disk->src->type == VIR_STORAGE_TYPE_NETWORK &&
+disk->src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD) {
 /* NB: If libvirt starts using the more modern option based
  * syntax to build the command line (e.g., "-drive driver=rbd,
  * filename=%s,...") instead of the legacy model (e.g."-drive
@@ -4892,22 +4900,36 @@ qemuBuildSCSIHostHostdevDrvStr(virDomainHostdevDefPtr 
dev)
 }
 
 static char *
-qemuBuildSCSIiSCSIHostdevDrvStr(virDomainHostdevDefPtr dev)
+qemuBuildSCSIiSCSIHostdevDrvStr(virDomainHostdevDefPtr dev,
+virQEMUCapsPtr qemuCaps)
 {
 char *source = NULL;
 char *netsource = NULL;
+virJSONValuePtr srcprops = NULL;
 virDomainHostdevSubsysSCSIPtr scsisrc = >source.subsys.u.scsi;
 virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc = >u.iscsi;
 qemuDomainStorageSourcePrivatePtr srcPriv =
 QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(iscsisrc->src);
 
-/* Rather than pull what we think we want - use the network disk code */
-netsource = qemuBuildNetworkDriveStr(iscsisrc->src, srcPriv ?
- srcPriv->secinfo : NULL);
-if (!netsource)
-goto cleanup;
-if (virAsprintf(, "file=%s,if=none,format=raw", netsource) < 0)
-goto cleanup;
+if (qemuDiskSourceNeedsProps(iscsisrc->src, qemuCaps)) {
+if (!(srcprops = qemuDiskSourceGetProps(iscsisrc->src))) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("failed to build the backend props"));
+goto cleanup;
+}
+
+if (!(netsource = virQEMUBuildDriveCommandlineFromJSON(srcprops)))
+goto cleanup;
+if (virAsprintf(, 

[libvirt] [REPOST PATCH v6 2/8] qemu: Use private storage source for iscsi instead of private hostdev

[John Ferlan]

Rather than placing/using privateData about secinfo in the hostdev,
let's use the virStorageSource private data instead.

Signed-off-by: John Ferlan 
---
 src/qemu/qemu_command.c |  7 ---
 src/qemu/qemu_domain.c  | 24 
 2 files changed, 20 insertions(+), 11 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index dc92fe9c7a..70a389f2a3 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4895,13 +4895,14 @@ static char *
 qemuBuildSCSIiSCSIHostdevDrvStr(virDomainHostdevDefPtr dev)
 {
 char *source = NULL;
-qemuDomainHostdevPrivatePtr hostdevPriv = QEMU_DOMAIN_HOSTDEV_PRIVATE(dev);
-
 virDomainHostdevSubsysSCSIPtr scsisrc = >source.subsys.u.scsi;
 virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc = >u.iscsi;
+qemuDomainStorageSourcePrivatePtr srcPriv =
+QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(iscsisrc->src);
 
 /* Rather than pull what we think we want - use the network disk code */
-source = qemuBuildNetworkDriveStr(iscsisrc->src, hostdevPriv->secinfo);
+source = qemuBuildNetworkDriveStr(iscsisrc->src, srcPriv ?
+  srcPriv->secinfo : NULL);
 
 return source;
 }
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 1abba02e42..31ec91fa12 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -1463,13 +1463,18 @@ qemuDomainSecretDiskPrepare(virConnectPtr conn,
 void
 qemuDomainSecretHostdevDestroy(virDomainHostdevDefPtr hostdev)
 {
-qemuDomainHostdevPrivatePtr hostdevPriv =
-QEMU_DOMAIN_HOSTDEV_PRIVATE(hostdev);
+qemuDomainStorageSourcePrivatePtr srcPriv;
 
-if (!hostdevPriv || !hostdevPriv->secinfo)
-return;
+if (virHostdevIsSCSIDevice(hostdev)) {
+virDomainHostdevSubsysSCSIPtr scsisrc = >source.subsys.u.scsi;
+virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc = >u.iscsi;
 
-qemuDomainSecretInfoFree(>secinfo);
+if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) {
+srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(iscsisrc->src);
+if (srcPriv && srcPriv->secinfo)
+qemuDomainSecretInfoFree(>secinfo);
+}
+}
 }
 
 
@@ -1491,14 +1496,17 @@ qemuDomainSecretHostdevPrepare(virConnectPtr conn,
 virDomainHostdevSubsysSCSIPtr scsisrc = >source.subsys.u.scsi;
 virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc = >u.iscsi;
 virStorageSourcePtr src = iscsisrc->src;
+qemuDomainStorageSourcePrivatePtr srcPriv;
 
 if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI &&
 src->auth) {
 
-qemuDomainHostdevPrivatePtr hostdevPriv =
-QEMU_DOMAIN_HOSTDEV_PRIVATE(hostdev);
+if (!(src->privateData = qemuDomainStorageSourcePrivateNew()))
+return -1;
+
+srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src);
 
-if (!(hostdevPriv->secinfo =
+if (!(srcPriv->secinfo =
   qemuDomainSecretInfoNew(conn, priv, hostdev->info->alias,
   VIR_SECRET_USAGE_TYPE_ISCSI,
   src->auth->username,
-- 
2.13.6

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

[libvirt] [REPOST PATCH v6 5/8] qemu: Get capabilities to use iscsi password-secret argument

[John Ferlan]

Add the capability to use the blockdev-add query-qmp-schema option
to find the 'password-secret' parameter that will allow the iSCSI
code to use the master secret object to encrypt the secret for an
and only need to provide the object id of the secret on the command
line thus obsfuscating the passphrase.

Signed-off-by: John Ferlan 
---
 src/qemu/qemu_capabilities.c | 2 ++
 src/qemu/qemu_capabilities.h | 1 +
 tests/qemucapabilitiesdata/caps_2.10.0-gicv2.aarch64.xml | 1 +
 tests/qemucapabilitiesdata/caps_2.10.0-gicv3.aarch64.xml | 1 +
 tests/qemucapabilitiesdata/caps_2.10.0.ppc64.xml | 1 +
 tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml | 1 +
 tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml| 1 +
 tests/qemucapabilitiesdata/caps_2.9.0.ppc64.xml  | 1 +
 tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml  | 1 +
 tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml | 1 +
 10 files changed, 11 insertions(+)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 7cb091056b..9d0c47fb2b 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -443,6 +443,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
   /* 270 */
   "vxhs",
   "virtio-blk.num-queues",
+  "iscsi.password-secret",
 );
 
 
@@ -1794,6 +1795,7 @@ static struct virQEMUCapsStringFlags 
virQEMUCapsQMPSchemaQueries[] = {
 { "blockdev-add/arg-type/options/+gluster/debug-level", 
QEMU_CAPS_GLUSTER_DEBUG_LEVEL},
 { "blockdev-add/arg-type/+gluster/debug", QEMU_CAPS_GLUSTER_DEBUG_LEVEL},
 { "blockdev-add/arg-type/+vxhs", QEMU_CAPS_VXHS},
+{ "blockdev-add/arg-type/+iscsi/password-secret", 
QEMU_CAPS_ISCSI_PASSWORD_SECRET },
 };
 
 struct virQEMUCapsObjectTypeProps {
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index cacc2b77ed..a35cea361d 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -429,6 +429,7 @@ typedef enum {
 /* 270 */
 QEMU_CAPS_VXHS, /* -drive file.driver=vxhs via query-qmp-schema */
 QEMU_CAPS_VIRTIO_BLK_NUM_QUEUES, /* virtio-blk-*.num-queues */
+QEMU_CAPS_ISCSI_PASSWORD_SECRET, /* -drive 
file.driver=iscsi,...,password-secret= */
 
 QEMU_CAPS_LAST /* this must always be the last item */
 } virQEMUCapsFlags;
diff --git a/tests/qemucapabilitiesdata/caps_2.10.0-gicv2.aarch64.xml 
b/tests/qemucapabilitiesdata/caps_2.10.0-gicv2.aarch64.xml
index 9f9dceb684..06723819c0 100644
--- a/tests/qemucapabilitiesdata/caps_2.10.0-gicv2.aarch64.xml
+++ b/tests/qemucapabilitiesdata/caps_2.10.0-gicv2.aarch64.xml
@@ -180,6 +180,7 @@
   
   
   
+  
   201
   0
(v2.10.0)
diff --git a/tests/qemucapabilitiesdata/caps_2.10.0-gicv3.aarch64.xml 
b/tests/qemucapabilitiesdata/caps_2.10.0-gicv3.aarch64.xml
index 3c2d2eed66..70f5a0d088 100644
--- a/tests/qemucapabilitiesdata/caps_2.10.0-gicv3.aarch64.xml
+++ b/tests/qemucapabilitiesdata/caps_2.10.0-gicv3.aarch64.xml
@@ -180,6 +180,7 @@
   
   
   
+  
   201
   0
(v2.10.0)
diff --git a/tests/qemucapabilitiesdata/caps_2.10.0.ppc64.xml 
b/tests/qemucapabilitiesdata/caps_2.10.0.ppc64.xml
index 0dfa20726c..855afac058 100644
--- a/tests/qemucapabilitiesdata/caps_2.10.0.ppc64.xml
+++ b/tests/qemucapabilitiesdata/caps_2.10.0.ppc64.xml
@@ -177,6 +177,7 @@
   
   
   
+  
   201
   0
(v2.10.0)
diff --git a/tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml 
b/tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml
index 7e44652feb..b340f8f96b 100644
--- a/tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml
+++ b/tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml
@@ -141,6 +141,7 @@
   
   
   
+  
   201
   0
   
diff --git a/tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml 
b/tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml
index ddbd8c32fa..9fb0515fdb 100644
--- a/tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml
@@ -224,6 +224,7 @@
   
   
   
+  
   201
   0
(v2.10.0)
diff --git a/tests/qemucapabilitiesdata/caps_2.9.0.ppc64.xml 
b/tests/qemucapabilitiesdata/caps_2.9.0.ppc64.xml
index 786cea8eab..e2bba89d40 100644
--- a/tests/qemucapabilitiesdata/caps_2.9.0.ppc64.xml
+++ b/tests/qemucapabilitiesdata/caps_2.9.0.ppc64.xml
@@ -173,6 +173,7 @@
   
   
   
+  
   2009000
   0
(v2.9.0)
diff --git a/tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml 
b/tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml
index 896ed503c3..4dc9ad5b56 100644
--- a/tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml
+++ b/tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml
@@ -138,6 +138,7 @@
   
   
   
+  
   2009000
   0
   
diff --git a/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml 
b/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml
index 05f9dc0308..0c6eb5046c 100644
--- a/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml

[libvirt] [REPOST PATCH v6 7/8] docs: Add news article regarding auth/encryption placement

[John Ferlan]

Signed-off-by: John Ferlan 
---
 docs/news.xml | 13 +
 1 file changed, 13 insertions(+)

diff --git a/docs/news.xml b/docs/news.xml
index 87391af856..9ec2780985 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -35,6 +35,19 @@
 
   
 
+  
+
+  conf: Move the auth and encryption definitions to disk source
+
+
+  Allow parsing and formatting of the auth and
+  encryption sub-elements to be a child of the
+  source element. This will allow adding an
+  auth sub-element to a backingStore
+  or mirror elements as a means to track specific
+  authentication and/or encryption needs.
+
+  
 
 
   
-- 
2.13.6

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

[libvirt] [REPOST PATCH v6 8/8] docs: Add news article to describe iSCSI usage of secret object

[John Ferlan]

Signed-off-by: John Ferlan 
---
 docs/news.xml | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/docs/news.xml b/docs/news.xml
index 9ec2780985..800067595f 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -137,6 +137,16 @@
   order of appearance.
 
   
+  
+
+  Securely pass iSCSI authentication data
+
+
+  Rather than supplying the authentication data as part of the
+  iSCSI URL for a disk or host device, utilize the encrypted
+  secret object to securely pass the authentication data.
+
+  
 
   
   
-- 
2.13.6

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

[libvirt] [REPOST PATCH v6 4/8] qemu: Refactor qemuBuildSCSIiSCSIHostdevDrvStr slightly

[John Ferlan]

Rather than building the "file" string in qemuBuildSCSIHostdevDrvStr
build it in the called helper.

Signed-off-by: John Ferlan 
---
 src/qemu/qemu_command.c | 13 ++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 70a389f2a3..577c76b44b 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4895,15 +4895,22 @@ static char *
 qemuBuildSCSIiSCSIHostdevDrvStr(virDomainHostdevDefPtr dev)
 {
 char *source = NULL;
+char *netsource = NULL;
 virDomainHostdevSubsysSCSIPtr scsisrc = >source.subsys.u.scsi;
 virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc = >u.iscsi;
 qemuDomainStorageSourcePrivatePtr srcPriv =
 QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(iscsisrc->src);
 
 /* Rather than pull what we think we want - use the network disk code */
-source = qemuBuildNetworkDriveStr(iscsisrc->src, srcPriv ?
-  srcPriv->secinfo : NULL);
+netsource = qemuBuildNetworkDriveStr(iscsisrc->src, srcPriv ?
+ srcPriv->secinfo : NULL);
+if (!netsource)
+goto cleanup;
+if (virAsprintf(, "file=%s,if=none,format=raw", netsource) < 0)
+goto cleanup;
 
+ cleanup:
+VIR_FREE(netsource);
 return source;
 }
 
@@ -4956,7 +4963,7 @@ qemuBuildSCSIHostdevDrvStr(virDomainHostdevDefPtr dev)
 if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) {
 if (!(source = qemuBuildSCSIiSCSIHostdevDrvStr(dev)))
 goto error;
-virBufferAsprintf(, "file=%s,if=none,format=raw", source);
+virBufferAsprintf(, "%s", source);
 } else {
 if (!(source = qemuBuildSCSIHostHostdevDrvStr(dev)))
 goto error;
-- 
2.13.6

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

[libvirt] [REPOST PATCH v6 3/8] qemu: Remove private hostdev

[John Ferlan]

Since it's not longer used to shuttle the @secinfo, let's remove
the private hostdev completely.

Signed-off-by: John Ferlan 
---
 src/conf/domain_conf.c| 12 ++--
 src/conf/domain_conf.h|  4 +---
 src/lxc/lxc_native.c  |  2 +-
 src/qemu/qemu_domain.c| 44 ---
 src/qemu/qemu_domain.h| 14 --
 src/qemu/qemu_parse_command.c |  4 ++--
 src/vbox/vbox_common.c|  2 +-
 src/xenconfig/xen_common.c|  2 +-
 src/xenconfig/xen_sxpr.c  |  2 +-
 src/xenconfig/xen_xl.c|  2 +-
 tests/virhostdevtest.c|  2 +-
 11 files changed, 11 insertions(+), 79 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 32b4cc9da9..9641295c57 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2451,7 +2451,7 @@ void virDomainVideoDefFree(virDomainVideoDefPtr def)
 
 
 virDomainHostdevDefPtr
-virDomainHostdevDefNew(virDomainXMLOptionPtr xmlopt)
+virDomainHostdevDefNew(void)
 {
 virDomainHostdevDefPtr def;
 
@@ -2461,11 +2461,6 @@ virDomainHostdevDefNew(virDomainXMLOptionPtr xmlopt)
 if (VIR_ALLOC(def->info) < 0)
 goto error;
 
-if (xmlopt &&
-xmlopt->privateData.hostdevNew &&
-!(def->privateData = xmlopt->privateData.hostdevNew()))
-goto error;
-
 return def;
 
  error:
@@ -2544,9 +2539,6 @@ void virDomainHostdevDefClear(virDomainHostdevDefPtr def)
 }
 break;
 }
-
-virObjectUnref(def->privateData);
-def->privateData = NULL;
 }
 
 void virDomainTPMDefFree(virDomainTPMDefPtr def)
@@ -14683,7 +14675,7 @@ virDomainHostdevDefParseXML(virDomainXMLOptionPtr 
xmlopt,
 
 ctxt->node = node;
 
-if (!(def = virDomainHostdevDefNew(xmlopt)))
+if (!(def = virDomainHostdevDefNew()))
 goto error;
 
 if (mode) {
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index c5a8d9b99a..5d01bb70d4 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -439,7 +439,6 @@ struct _virDomainHostdevCaps {
 /* basic device for direct passthrough */
 struct _virDomainHostdevDef {
 virDomainDeviceDef parent; /* higher level Def containing this */
-virObjectPtr privateData;
 
 int mode; /* enum virDomainHostdevMode */
 int startupPolicy; /* enum virDomainStartupPolicy */
@@ -2607,7 +2606,6 @@ struct _virDomainXMLPrivateDataCallbacks {
 /* note that private data for devices are not copied when using
  * virDomainDefCopy and similar functions */
 virDomainXMLPrivateDataNewFuncdiskNew;
-virDomainXMLPrivateDataNewFunchostdevNew;
 virDomainXMLPrivateDataNewFuncvcpuNew;
 virDomainXMLPrivateDataNewFuncchrSourceNew;
 virDomainXMLPrivateDataFormatFunc format;
@@ -2732,7 +2730,7 @@ void virDomainNVRAMDefFree(virDomainNVRAMDefPtr def);
 void virDomainWatchdogDefFree(virDomainWatchdogDefPtr def);
 virDomainVideoDefPtr virDomainVideoDefNew(void);
 void virDomainVideoDefFree(virDomainVideoDefPtr def);
-virDomainHostdevDefPtr virDomainHostdevDefNew(virDomainXMLOptionPtr xmlopt);
+virDomainHostdevDefPtr virDomainHostdevDefNew(void);
 void virDomainHostdevDefClear(virDomainHostdevDefPtr def);
 void virDomainHostdevDefFree(virDomainHostdevDefPtr def);
 void virDomainHubDefFree(virDomainHubDefPtr def);
diff --git a/src/lxc/lxc_native.c b/src/lxc/lxc_native.c
index 68636dc2a4..fdc03a57ea 100644
--- a/src/lxc/lxc_native.c
+++ b/src/lxc/lxc_native.c
@@ -394,7 +394,7 @@ lxcCreateNetDef(const char *type,
 static virDomainHostdevDefPtr
 lxcCreateHostdevDef(int mode, int type, const char *data)
 {
-virDomainHostdevDefPtr hostdev = virDomainHostdevDefNew(NULL);
+virDomainHostdevDefPtr hostdev = virDomainHostdevDefNew();
 
 if (!hostdev)
 return NULL;
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 31ec91fa12..66fa069b6f 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -965,49 +965,6 @@ qemuDomainStorageSourcePrivateDispose(void *obj)
 }
 
 
-static virClassPtr qemuDomainHostdevPrivateClass;
-static void qemuDomainHostdevPrivateDispose(void *obj);
-
-static int
-qemuDomainHostdevPrivateOnceInit(void)
-{
-qemuDomainHostdevPrivateClass =
-virClassNew(virClassForObject(),
-"qemuDomainHostdevPrivate",
-sizeof(qemuDomainHostdevPrivate),
-qemuDomainHostdevPrivateDispose);
-if (!qemuDomainHostdevPrivateClass)
-return -1;
-else
-return 0;
-}
-
-VIR_ONCE_GLOBAL_INIT(qemuDomainHostdevPrivate)
-
-static virObjectPtr
-qemuDomainHostdevPrivateNew(void)
-{
-qemuDomainHostdevPrivatePtr priv;
-
-if (qemuDomainHostdevPrivateInitialize() < 0)
-return NULL;
-
-if (!(priv = virObjectNew(qemuDomainHostdevPrivateClass)))
-return NULL;
-
-return (virObjectPtr) priv;
-}
-
-
-static void
-qemuDomainHostdevPrivateDispose(void *obj)
-{
-qemuDomainHostdevPrivatePtr priv = obj;

[libvirt] [REPOST PATCH v6 1/8] conf, qemu: Replace iscsisrc fields with virStorageSourcePtr

[John Ferlan]

Rather than picking apart the two pieces we need/want (path, hosts,
and auth)- let's allocate/use a virStorageSourcePtr for iSCSI storage.

The end result is that qemuBuildSCSIiSCSIHostdevDrvStr doesn't need
to "fake" one for the qemuBuildNetworkDriveStr call.

Signed-off-by: John Ferlan 
---
 src/conf/domain_conf.c  | 46 +-
 src/conf/domain_conf.h  |  5 +
 src/qemu/qemu_command.c | 10 +-
 src/qemu/qemu_domain.c  |  9 +
 src/qemu/qemu_hotplug.c |  2 +-
 5 files changed, 33 insertions(+), 39 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 7dfd7b54e6..32b4cc9da9 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2480,10 +2480,9 @@ 
virDomainHostdevSubsysSCSIiSCSIClear(virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc
 {
 if (!iscsisrc)
 return;
-VIR_FREE(iscsisrc->path);
-virStorageNetHostDefFree(iscsisrc->nhosts, iscsisrc->hosts);
-virStorageAuthDefFree(iscsisrc->auth);
-iscsisrc->auth = NULL;
+
+virStorageSourceFree(iscsisrc->src);
+iscsisrc->src = NULL;
 }
 
 
@@ -4357,7 +4356,7 @@ virDomainHostdevDefPostParse(virDomainHostdevDefPtr dev,
 if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) {
 virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc = >u.iscsi;
 
-if (virDomainPostParseCheckISCSIPath(>path) < 0)
+if (virDomainPostParseCheckISCSIPath(>src->path) < 0)
 return -1;
 }
 
@@ -7123,24 +7122,29 @@ virDomainHostdevSubsysSCSIiSCSIDefParseXML(xmlNodePtr 
sourcenode,
 virStorageAuthDefPtr authdef = NULL;
 virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc = >u.iscsi;
 
-/* Similar to virDomainDiskSourceParse for a VIR_STORAGE_TYPE_NETWORK */
+/* For the purposes of command line creation, this needs to look
+ * like a disk storage source */
+if (VIR_ALLOC(iscsisrc->src) < 0)
+return -1;
+iscsisrc->src->type = VIR_STORAGE_TYPE_NETWORK;
+iscsisrc->src->protocol = VIR_STORAGE_NET_PROTOCOL_ISCSI;
 
-if (!(iscsisrc->path = virXMLPropString(sourcenode, "name"))) {
+if (!(iscsisrc->src->path = virXMLPropString(sourcenode, "name"))) {
 virReportError(VIR_ERR_XML_ERROR, "%s",
_("missing iSCSI hostdev source path name"));
 goto cleanup;
 }
 
-if (virDomainStorageNetworkParseHosts(sourcenode, >hosts,
-  >nhosts) < 0)
+if (virDomainStorageNetworkParseHosts(sourcenode, >src->hosts,
+  >src->nhosts) < 0)
 goto cleanup;
 
-if (iscsisrc->nhosts < 1) {
+if (iscsisrc->src->nhosts < 1) {
 virReportError(VIR_ERR_XML_ERROR, "%s",
_("missing the host address for the iSCSI hostdev"));
 goto cleanup;
 }
-if (iscsisrc->nhosts > 1) {
+if (iscsisrc->src->nhosts > 1) {
 virReportError(VIR_ERR_XML_ERROR, "%s",
_("only one source host address may be specified "
  "for the iSCSI hostdev"));
@@ -7166,7 +7170,7 @@ virDomainHostdevSubsysSCSIiSCSIDefParseXML(xmlNodePtr 
sourcenode,
authdef->secrettype);
 goto cleanup;
 }
-iscsisrc->auth = authdef;
+iscsisrc->src->auth = authdef;
 authdef = NULL;
 }
 cur = cur->next;
@@ -15677,9 +15681,9 @@ 
virDomainHostdevMatchSubsysSCSIiSCSI(virDomainHostdevDefPtr first,
 virDomainHostdevSubsysSCSIiSCSIPtr second_iscsisrc =
 >source.subsys.u.scsi.u.iscsi;
 
-if (STREQ(first_iscsisrc->hosts[0].name, second_iscsisrc->hosts[0].name) &&
-first_iscsisrc->hosts[0].port == second_iscsisrc->hosts[0].port &&
-STREQ(first_iscsisrc->path, second_iscsisrc->path))
+if (STREQ(first_iscsisrc->src->hosts[0].name, 
second_iscsisrc->src->hosts[0].name) &&
+first_iscsisrc->src->hosts[0].port == 
second_iscsisrc->src->hosts[0].port &&
+STREQ(first_iscsisrc->src->path, second_iscsisrc->src->path))
 return 1;
 return 0;
 }
@@ -23028,7 +23032,7 @@ virDomainHostdevDefFormatSubsys(virBufferPtr buf,
 virDomainHostdevSubsysSCSIProtocolTypeToString(scsisrc->protocol);
 
 virBufferAsprintf(buf, " protocol='%s' name='%s'",
-  protocol, iscsisrc->path);
+  protocol, iscsisrc->src->path);
 }
 
 if (def->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI_HOST) {
@@ -23080,9 +23084,9 @@ virDomainHostdevDefFormatSubsys(virBufferPtr buf,
 case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI:
 if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) {
 virBufferAddLit(buf, "hosts[0].name);
-if (iscsisrc->hosts[0].port)
-

[libvirt] [REPOST PATCH v6 0/8] Use secret objects to pass iSCSI passwords

[John Ferlan]

Repost after updating the series to account for Peter's most recent
-drive and blockdev-add saga changes.

Includes adding the port number to the .args output and updating
the *.xmlfor the newly added ppc and gic*-aarch64 capabilitiesdata 

Also merge the news.xml changes w/ recent vbox changes.

Previous repost here:
https://www.redhat.com/archives/libvir-list/2017-November/msg00124.html

Original:
https://www.redhat.com/archives/libvir-list/2017-October/msg01012.html

Copy of Original cover letter:

v5: https://www.redhat.com/archives/libvir-list/2017-October/msg00228.html

FWIW: AFAICT this series does not need/require changes that Peter has
  posted for continuing blocked-add saga related to user-specified
  backing chains:

https://www.redhat.com/archives/libvir-list/2017-October/msg00956.html

Changes since v5:

 * Some patches pushed as part of Peter Krempa's work and the two patches
   to perform the parsing of auth and encryption data in virStorageSource
   were pushed separately

 * Removed patches dealing with qemuDomainStorageSourceCopy and
   virDomainDiskStorageSourceNew

 Patches 9->16 reworked

 Patch1: (previous patch 10)
   - Rework logic to remove the need to pass around the @xmlopt for
 virStorageSourcePtr allocation and instead VIR_ALLOC(iscsisrc->src)
 directly as other consumers do.

 Patch2: (previous patch 11)
   - Use the qemuDomainStorageSourcePrivatePtr and friend macro
 QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE to manage the iscsisrc->src data
 making sure to ensure that srcPriv (e.g. privateData) exists.

 Patch3: (previous patch 12)
   - Only minor merge related changes.

 Patch4: (previous patch 13)
   - Merge related plus additional check to ensure srcPriv exists before
 dereference secinfo

 Patch5: (previous patch 14)
   - No change

 Patch 6: (previous patch 15)
   - Merge related changes, plus checks for srcPriv before deref secinfo

   NB: Testing note - I did ensure at this point if the password secret
   capability check fails that the code will still do the right thing.

 Patch7: (previous patch 9)
   - No change... Presented since it wasn't ACK'd before

 Patch8: (previous patch16)
   - No change


John Ferlan (8):
  conf,qemu: Replace iscsisrc fields with virStorageSourcePtr
  qemu: Use private storage source for iscsi instead of private hostdev
  qemu: Remove private hostdev
  qemu: Refactor qemuBuildSCSIiSCSIHostdevDrvStr slightly
  qemu: Get capabilities to use iscsi password-secret argument
  qemu: Use secret objects to pass iSCSI passwords
  docs: Add news article regarding auth/encryption placement
  docs: Add news article to describe iSCSI usage of secret object

 docs/news.xml  | 23 ++
 src/conf/domain_conf.c | 58 
 src/conf/domain_conf.h |  9 +--
 src/lxc/lxc_native.c   |  2 +-
 src/qemu/qemu_capabilities.c   |  2 +
 src/qemu/qemu_capabilities.h   |  1 +
 src/qemu/qemu_command.c| 75 +++-
 src/qemu/qemu_command.h|  3 +-
 src/qemu/qemu_domain.c | 81 +++---
 src/qemu/qemu_domain.h | 14 
 src/qemu/qemu_hotplug.c| 52 +-
 src/qemu/qemu_parse_command.c  |  4 +-
 src/vbox/vbox_common.c |  2 +-
 src/xenconfig/xen_common.c |  2 +-
 src/xenconfig/xen_sxpr.c   |  2 +-
 src/xenconfig/xen_xl.c |  2 +-
 .../caps_2.10.0-gicv2.aarch64.xml  |  1 +
 .../caps_2.10.0-gicv3.aarch64.xml  |  1 +
 tests/qemucapabilitiesdata/caps_2.10.0.ppc64.xml   |  1 +
 tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml   |  1 +
 tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml  |  1 +
 tests/qemucapabilitiesdata/caps_2.9.0.ppc64.xml|  1 +
 tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml|  1 +
 tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml   |  1 +
 ...xml2argv-disk-drive-network-iscsi-auth-AES.args | 41 +++
 ...uxml2argv-disk-drive-network-iscsi-auth-AES.xml | 43 
 ...ml2argv-hostdev-scsi-virtio-iscsi-auth-AES.args | 45 
 ...xml2argv-hostdev-scsi-virtio-iscsi-auth-AES.xml | 48 +
 tests/qemuxml2argvtest.c   | 10 +++
 tests/virhostdevtest.c |  2 +-
 30 files changed, 390 insertions(+), 139 deletions(-)
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-AES.args
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-AES.xml
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth-AES.args
 create mode 100644 

Re: [libvirt] [PATCH 11/12] qemu: block: Add node-names to JSON backing storage strings

[John Ferlan]

On 11/03/2017 10:29 AM, Peter Krempa wrote:
> Format out the node-name if it was assigned for JSON-based storage
> specification.
> ---
>  src/qemu/qemu_block.c | 5 +
>  1 file changed, 5 insertions(+)
> 

And Coverity also notes that for virStorageType) actualType ==
VIR_STORAGE_TYPE_VOLUME ((NONE and LAST, too), that fileprops will be
NULL and thus the virJSONValueObjectAdd won't be happy...

John

> diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
> index 6f6d294bf..6df0dc0fb 100644
> --- a/src/qemu/qemu_block.c
> +++ b/src/qemu/qemu_block.c
> @@ -1005,5 +1005,10 @@ 
> qemuBlockStorageSourceGetBackendProps(virStorageSourcePtr src)
>  break;
>  }
> 
> +if (virJSONValueObjectAdd(fileprops, "S:node-name", src->nodestorage, 
> NULL) < 0) {
> +virJSONValueFree(fileprops);
> +return NULL;
> +}
> +
>  return fileprops;
>  }
> 

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

Re: [libvirt] [PATCH 07/12] qemu: block: Add JSON props generator for NBD storage backing

[John Ferlan]

On 11/03/2017 10:29 AM, Peter Krempa wrote:
> ---
>  src/qemu/qemu_block.c | 32 
>  1 file changed, 32 insertions(+)
> 
> diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
> index 4e588c724..451d04694 100644
> --- a/src/qemu/qemu_block.c
> +++ b/src/qemu/qemu_block.c
> @@ -767,6 +767,34 @@ qemuBlockStorageSourceGetISCSIProps(virStorageSourcePtr 
> src)
>  }
> 
> 
> +static virJSONValuePtr
> +qemuBlockStorageSourceGetNBDProps(virStorageSourcePtr src)
> +{
> +virJSONValuePtr serverprops;
> +virJSONValuePtr ret = NULL;
> +
> +if (src->nhosts != 1) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +   _("nbd protocol accepts only one host"));
> +return NULL;
> +}
> +
> +serverprops = 
> qemuBlockStorageSourceBuildJSONSocketAddress(>hosts[0],
> +   false);
> +if (!serverprops)
> +return NULL;
> +
> +ignore_value(virJSONValueObjectCreate(,
> +  "s:driver", "nbd",
> +  "a:server", serverprops,
> +  "S:export", src->path,
> +  "S:tls-creds", src->tlsAlias,
> +  NULL));

Coverity notes that serverprops will be leaked on failure

Actually it REALLY doesn't like the "a:server" consumption, but I
already have quite a few of those in my false positive list.

John

> +
> +return ret;
> +}
> +
> +
>  /**
>   * qemuBlockStorageSourceGetBackendProps:
>   * @src: disk source
> @@ -822,6 +850,10 @@ 
> qemuBlockStorageSourceGetBackendProps(virStorageSourcePtr src)
>  break;
> 
>  case VIR_STORAGE_NET_PROTOCOL_NBD:
> +if (!(fileprops = qemuBlockStorageSourceGetNBDProps(src)))
> +return NULL;
> +break;
> +
>  case VIR_STORAGE_NET_PROTOCOL_RBD:
>  case VIR_STORAGE_NET_PROTOCOL_SHEEPDOG:
>  case VIR_STORAGE_NET_PROTOCOL_SSH:
> 

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

Re: [libvirt] [PATCH 09/12] qemu: block: Add JSON props generator for sheepdog storage backing

[John Ferlan]

On 11/03/2017 10:29 AM, Peter Krempa wrote:
> ---
>  src/qemu/qemu_block.c | 31 +++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
> index 8a1ce8262..5f28c4dd6 100644
> --- a/src/qemu/qemu_block.c
> +++ b/src/qemu/qemu_block.c
> @@ -865,6 +865,33 @@ qemuBlockStorageSourceGetRBDProps(virStorageSourcePtr 
> src)
>  }
> 
> 
> +static virJSONValuePtr
> +qemuBlockStorageSourceGetSheepdogProps(virStorageSourcePtr src)
> +{
> +virJSONValuePtr serverprops;
> +virJSONValuePtr ret = NULL;
> +
> +if (src->nhosts != 1) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +   _("sheepdog protocol accepts only one host"));
> +return NULL;
> +}
> +
> +serverprops = 
> qemuBlockStorageSourceBuildJSONSocketAddress(>hosts[0],
> +   false);
> +if (!serverprops)
> +return NULL;
> +
> +/* libvirt does not support the 'snap-id' and 'tag' properties */
> +ignore_value(virJSONValueObjectCreate(,
> +  "s:driver", "sheepdog",
> +  "a:server", serverprops,
> +  "s:vdi", src->path,
> +  NULL));

Again Coverity here w/ serverprops being leaked on failure.

John

> +
> +return ret;
> +}
> +
>  /**
>   * qemuBlockStorageSourceGetBackendProps:
>   * @src: disk source
> @@ -930,6 +957,10 @@ 
> qemuBlockStorageSourceGetBackendProps(virStorageSourcePtr src)
>  break;
> 
>  case VIR_STORAGE_NET_PROTOCOL_SHEEPDOG:
> +if (!(fileprops = qemuBlockStorageSourceGetSheepdogProps(src)))
> +return NULL;
> +break;
> +
>  case VIR_STORAGE_NET_PROTOCOL_SSH:
>  case VIR_STORAGE_NET_PROTOCOL_NONE:
>  case VIR_STORAGE_NET_PROTOCOL_LAST:
> 

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

Re: [libvirt] [PATCH] util: storage: Fix parsing of IPv6 portal address for iSCSI

[Jiri Denemark]

On Tue, Nov 07, 2017 at 16:26:51 +0100, Peter Krempa wrote:
> Split on the last colon and avoid parsing port if the split remainder
> contains the closing square bracket, so that IPv6 addresses are
> interpreted correctly.
> ---
>  src/util/virstoragefile.c |  3 ++-
>  tests/virstoragetest.c| 20 
>  2 files changed, 22 insertions(+), 1 deletion(-)

ACK

Jirka

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

Re: [libvirt] [PATCH go-xml] Add support for host paravirt bootloader, used by Xen and bhyve.

[Daniel P. Berrange]

On Tue, Nov 07, 2017 at 02:17:41PM -0600, Brandon Bergren wrote:
> ---
>  domain.go  |  2 ++
>  domain_test.go | 16 
>  2 files changed, 18 insertions(+)

THanks, pushed to git master


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

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

Re: [libvirt] [PATCH 3/4] tests: Rename ppc64le caps to ppc64

[Andrea Bolognani]

On Tue, 2017-11-07 at 17:47 -0500, John Ferlan wrote:
> So this is just for our tests right?  I guess I see a name change like
> this and I think - is there some sort of migrate or save/restore issue
> we could have by changing the "name"...

Nah, it's pretty much just the names of the files in the test suite
bothering me :)

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH 03/11] vsh: Add @silent to vshConnectionHook

[Martin Kletzander]

On Tue, Nov 07, 2017 at 01:22:51PM +0100, Michal Privoznik wrote:

In near future we will want to not report error when connecting
fails. In order to achieve that we have to pass a boolean that
suppresses error messages.

Signed-off-by: Michal Privoznik 
---
tools/virsh-domain.c |  2 +-
tools/virsh.c| 67 
tools/virsh.h|  5 +++-
tools/virt-admin.c   | 49 +-
tools/vsh.c  |  2 +-
tools/vsh.h  |  3 ++-
6 files changed, 76 insertions(+), 52 deletions(-)

diff --git a/tools/virsh.c b/tools/virsh.c
index d0c135016..7d6dc2620 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -191,15 +191,19 @@ virshConnect(vshControl *ctl, const char *uri, bool 
readonly)
if (interval > 0 &&
virConnectSetKeepAlive(c, interval, count) != 0) {
if (keepalive_forced) {
-vshError(ctl, "%s",
- _("Cannot setup keepalive on connection "
-   "as requested, disconnecting"));
+if (!silent) {
+vshError(ctl, "%s",
+ _("Cannot setup keepalive on connection "
+   "as requested, disconnecting"));
+}
virConnectClose(c);
c = NULL;
goto cleanup;
}
-vshDebug(ctl, VSH_ERR_INFO, "%s",
- _("Failed to setup keepalive on connection\n"));
+if (!silent) {
+vshDebug(ctl, VSH_ERR_INFO, "%s",
+ _("Failed to setup keepalive on connection\n"));
+}


I guess debug messages probably need to be filtered too.  Actually
allocation errors should be covered as well.  And you missed some.  It's
fine as it is, since no auto-completion is perfect, I use lot of them
extensively and I must say I don't care if it outputs something when
some error happens.  The nice thing about auto-completion is that if it
does not work at all or works differently (outputs something it
"shouldn't") nothing happens.  There is no data loss, broken migrations
or whatever.

Anyway looking through all the things that you are modifying here and to
those that could still be modified, I think this could be approached a
bit better.  As I said it's kind of fine if we keep it like this for
now, but it could be even better.  Consider this:

- we do not modify what messages are reported
- we leave the function that manages log output to decide
- when the completer is called we set it up so that there is no log
  output

Guess what, we have first two things in place and for the third one you
can just call vshCloseLogFile(ctl) and you're golden ;)

If you want to make it even better, you can already do that before you
get to any completer.  For example if you specify '-q' multiple times on
the command line it could switch to super quiet mode, e.g.:

diff --git i/tools/virsh.c w/tools/virsh.c
index f830331f6bbe..6b7eafeba0be 100644
--- i/tools/virsh.c
+++ w/tools/virsh.c
@@ -782,6 +782,8 @@ virshParseArgv(vshControl *ctl, int argc, char **argv)
vshOpenLogFile(ctl);
break;
case 'q':
+if (ctl->quiet)
+vshCloseLogFile(ctl);
ctl->quiet = true;
break;
case 't':
--

And just call virsh from the bash completion with '-qq'

And the same for virt-admin of course.  And maybe document it :)


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

Re: [libvirt] [PATCH 02/11] vshCommandOpt: Relax check for valid options

[Martin Kletzander]

On Tue, Nov 07, 2017 at 01:22:50PM +0100, Michal Privoznik wrote:

When trying to get an opt for command typed on the command line
we first check if command has such option. Because if it doesn't
it is a programming error. For instance: vshCommandOptBool(cmd,
"config") called from say cmdStart() doesn't make sense since
there's no --config for the start command. However, we will want
to have generic completers which are going to check if various
options are set. And so it can happen that we will check for
non-existent option for given command. Therefore, we need to
relax the check otherwise we will hit the assert() and don't get
anywhere.



I would prefer keeping the assert there since it's such an easy check
for that kind of programming error.  Is there no way to distinguish
between calls from the completer?  If not, then I would rename it to
vshCommandOptInternal(), add a bool argument, make vshCommandOpt() call
it with one value and the completer with another one (I don't care if
there's yet another function for that or if completers use the Internal
one).


Signed-off-by: Michal Privoznik 
---
tools/vsh.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/tools/vsh.c b/tools/vsh.c
index eca312b4b..24ea45aa4 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -815,8 +815,7 @@ vshCommandFree(vshCmd *cmd)
 * Look up an option passed to CMD by NAME.  Returns 1 with *OPT set
 * to the option if found, 0 with *OPT set to NULL if the name is
 * valid and the option is not required, -1 with *OPT set to NULL if
- * the option is required but not present, and assert if NAME is not
- * valid (which indicates a programming error).  No error messages are
+ * the option is required but not present. No error messages are
 * issued if a value is returned.
 */
static int
@@ -829,8 +828,7 @@ vshCommandOpt(const vshCmd *cmd, const char *name, 
vshCmdOpt **opt,

/* See if option is valid and/or required.  */
*opt = NULL;
-while (valid) {
-assert(valid->name);
+while (valid && valid->name) {
if (STREQ(name, valid->name))
break;
valid++;
--
2.13.6

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


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

Re: [libvirt] [PATCH v3 0/5] Predictable file names for memory-backend-file

[Michal Privoznik]

On 11/07/2017 10:45 PM, John Ferlan wrote:
> 
> 
> On 11/07/2017 10:50 AM, Michal Privoznik wrote:
>> v3 of:
>>
>> https://www.redhat.com/archives/libvir-list/2017-October/msg01091.html
>>
>> diff to v2:
>> - Pushed 1/4 and 2/4 from the original series
>> - Split 3/4 into smaller patches
>>
>> Michal Privoznik (5):
>>   qemu: Set alias for memory cell in qemuBuildMemoryCellBackendStr
>>   qemu: Rename qemuProcessBuildDestroyHugepagesPath
>>   qemu: Destroy whole memory tree
>>   qemu: Use predictable file names for memory-backend-file
>>   news: Document predictable file names for memory-backend-file
>>
>>  docs/news.xml  |  11 ++
>>  src/qemu/qemu_command.c|   9 +-
>>  src/qemu/qemu_conf.c   |  55 -
>>  src/qemu/qemu_conf.h   |   9 +-
>>  src/qemu/qemu_driver.c |  17 +++
>>  src/qemu/qemu_hotplug.c|   2 +-
>>  src/qemu/qemu_process.c| 137 
>> +++--
>>  src/qemu/qemu_process.h|   8 +-
>>  .../qemuxml2argv-cpu-numa-memshared.args   |   6 +-
>>  .../qemuxml2argv-fd-memory-numa-topology.args  |   3 +-
>>  .../qemuxml2argv-fd-memory-numa-topology2.args |   6 +-
>>  .../qemuxml2argv-fd-memory-numa-topology3.args |   9 +-
>>  .../qemuxml2argv-hugepages-memaccess2.args |   9 +-
>>  13 files changed, 218 insertions(+), 63 deletions(-)
>>
> 
> There's a couple of things to clean up, but in general with those
> cleanups in place,
> 
> Reviewed-by: John Ferlan 

Thanks. Fixed all the raised points and pushed.

Michal

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

Re: [libvirt] [PATCH] virconf: properly set the end of content

[Daniel P. Berrange]

On Tue, Nov 07, 2017 at 03:37:46PM -0700, Jim Fehlig wrote:
> There was a recent report of the xen-xl converter not handling
> config files missing an ending newline
> 
> https://www.redhat.com/archives/libvir-list/2017-October/msg01353.html
> 
> Commit 3cc2a9e0 fixed a similar problem when parsing content of a
> file but missed parsing in-memory content. But AFAICT, the better
> fix is to properly set the end of the content when initializing the
> virConfParserCtxt in virConfParse().
> 
> This commit reverts the part of 3cc2a9e0 that appends a newline to
> files missing it, and fixes setting the end of content when
> initializing virConfParserCtxt.
> 
> Signed-off-by: Jim Fehlig 
> ---
>  src/util/virconf.c | 13 ++---
>  1 file changed, 2 insertions(+), 11 deletions(-)

Could you try to extend  tests/virconftest.c to cover in-memory
content for this newline scenario too.

> 
> diff --git a/src/util/virconf.c b/src/util/virconf.c
> index 39c2bd917..a82a509ca 100644
> --- a/src/util/virconf.c
> +++ b/src/util/virconf.c
> @@ -705,7 +705,7 @@ virConfParse(const char *filename, const char *content, 
> int len,
>  
>  ctxt.filename = filename;
>  ctxt.base = ctxt.cur = content;
> -ctxt.end = content + len - 1;
> +ctxt.end = content + len;
>  ctxt.line = 1;
>  
>  ctxt.conf = virConfCreate(filename, flags);
> @@ -745,7 +745,7 @@ virConfReadFile(const char *filename, unsigned int flags)
>  {
>  char *content;
>  int len;
> -virConfPtr conf = NULL;
> +virConfPtr conf;
>  
>  VIR_DEBUG("filename=%s", NULLSTR(filename));
>  
> @@ -757,17 +757,8 @@ virConfReadFile(const char *filename, unsigned int flags)
>  if ((len = virFileReadAll(filename, MAX_CONFIG_FILE_SIZE, )) < 0)
>  return NULL;
>  
> -if (len && len < MAX_CONFIG_FILE_SIZE && content[len - 1] != '\n') {
> -VIR_DEBUG("appending newline to busted config file %s", filename);
> -if (VIR_REALLOC_N(content, len + 2) < 0)
> -goto cleanup;
> -content[len++] = '\n';
> -content[len] = '\0';
> -}
> -
>  conf = virConfParse(filename, content, len, flags);
>  
> - cleanup:
>  VIR_FREE(content);
>  
>  return conf;
> -- 
> 2.14.2
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

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

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