Re: [libvirt] [PATCH] qemuDomainSaveMemory: Don't enforce dynamicOwnership

[Michal Prívozník]

On 07/02/2018 04:49 PM, John Ferlan wrote:
> 
> 
> On 06/26/2018 09:57 AM, Michal Privoznik wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1589115
>>
>> When doing a memory snapshot qemuOpenFile() is used. This means
>> that the file where memory is saved is firstly attempted to be
>> created under root:root (because that's what libvirtd is running
>> under) and if this fails the second attempt is done under
>> domain's uid:gid. This does not make much sense - qemu is given
>> opened FD so it does not need to access the file. Moreover, if
>> dynamicOwnership is set in qemu.conf and the file lives on a
>> squashed NFS this is deadly combination and very likely to fail.
>>
>> The fix consists of using:
>>
>>   qemuOpenFileAs(fallback_uid = cfg->user,
>>  fallback_gid = cfg->group,
>>  dynamicOwnership = false)
>>
>> In other words, dynamicOwnership is turned off for memory
>> snapshot (chown() will still be attempted if the file does not
>> live on NFS) and instead of using domain DAC label, configured
>> user:group is set as fallback.
>>
>> Signed-off-by: Michal Privoznik 
>> ---
>>  src/qemu/qemu_driver.c | 9 ++---
>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>
> 
> I see from reading the bz in a way you hoped to provoke upstream
> discussion on this, so...
> 
> It seems the primary motivation is to go with dynamicOwnership = false
> is because that "fixes" the bz for the "corner case" where someone is
> saving their snapshot to a root squashed NFS share. 

Yes.

> If the same scenario
> was played out for core dump or managed save image, wouldn't the same
> problem exist?

Ah, good catch. So managed save uses qemuDomainSaveMemory() which I'm
fixing here. But coredump uses doCoreDump() which needs the same treatment.

>  Although the latter would skip the O_CREAT check in
> qemuOpenFlagsAs thus not setting VIR_FILE_OPEN_FORCE_OWNER, but could
> perhaps theoretically fail as IIRC the root squash case only had one way
> to resolve.
> 
> Since @path_shared == 1 (only way to get to the second attempt to
> virFileOpenAs), then why doesn't this code try the VIR_FILE_OPEN_FORK?
> It's been a while, but IIRC that's what allowed storageBackendCreateRaw
> to be successful.

It does, but with fallback UID:GID (which is currently whatever DAC
label domain has). But at the same time chown() to the fallback UID:GID
pair is enforced because dynamicOwnership is set. And it is actually the
chown() that fails.

Now, as absurd as it may sound, we don't need the memory image to be the
same UID:GID as the running domain. Qemu is given a FD to migrate to (at
which point perms are not checked anymore), so for instance even an
unprivileged qemu process can write into a file owned by root:root.

> 
> IIUC the proposed solution to clear dynamicOwnership is purely to avoid
> setting VIR_FILE_OPEN_FORCE_OWNER since @path_shared == 1 in the O_CREAT
> path.

Exactly.

> 
> On a secondary note, this is the only path that doesn't pass NULL for
> bypassSecuritydriver... If you follow that bypass - all that really
> happens is it can alter the returned pointer, but that value is never
> checked in the code - so what is it's purpose?  Taking a quick look
> through history, I wonder if 23087cfdb was the last real consumer.

Ah, good point. I'll post a patch to remove it.

Michal

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

Re: [libvirt] [PATCH 5/5] virISCSIScanTargets: Allow making targets persistent

[Michal Prívozník]

On 07/03/2018 01:40 AM, John Ferlan wrote:
> 
> 
> On 06/29/2018 11:01 AM, Michal Privoznik wrote:
>> After new iSCSI interface is successfully set up, we issue
> 
> s/new/a new/
> s/issue/issue a/
> 
>> sendtargets command. However, after 56057900dc53df490d we don't
>> update the host config which in turn makes login fail because
>> iscsiadm is unable to find any matching record for the interface.
>>
>> Signed-off-by: Michal Privoznik 
>> ---
>>  src/storage/storage_backend_iscsi.c |  1 +
>>  src/util/viriscsi.c | 21 ++---
>>  src/util/viriscsi.h |  1 +
>>  tests/viriscsitest.c|  3 ++-
>>  4 files changed, 22 insertions(+), 4 deletions(-)
>>
> 
> Like the previous patch - is there a specific bug or something that led
> you down this path?  Can you show an example of the existing code that's
> creating a bad command line and generating an error and then how this
> fixes things.  It's not like we have tests and for this stuff it's
> really nice to have plenty of examples.

So here is the run without my patches:

debug : virCommandRunAsync:2476 : About to run iscsiadm --mode session
iscsiadm: No active sessions.
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal 
$PORTAL:3260,1 --targetname $TARGET --op new
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal 
$PORTAL:3260,1 --target $TARGET --op update --name node.session.auth.authmethod 
--value CHAP
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal 
$PORTAL:3260,1 --target $TARGET --op update --name node.session.auth.username 
--value $USER
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal 
$PORTAL:3260,1 --target $TARGET --op update --name node.session.auth.password 
--value $PASS
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode iface
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode iface 
--interface libvirt-iface-03316143 --op new
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode iface 
--interface libvirt-iface-03316143 --op update --name iface.initiatorname 
--value $INITIATOR
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode iface
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode discovery --type 
sendtargets --portal $PORTAL:3260,1 --op nonpersistent
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal 
$PORTAL:3260,1 --targetname $TARGET --login --interface libvirt-iface-03316143
error : virCommandWait:2600 : internal error: Child process (iscsiadm --mode 
node --portal $PORTAL:3260,1 --targetname $TARGET --login --interface 
libvirt-iface-03316143) unexpected exit status 21:
iscsiadm: No records found
iscsiadm: No records found


And with my patches:

debug : virCommandRunAsync:2476 : About to run iscsiadm --mode session
iscsiadm: No active sessions.
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal 
$PORTAL:3260,1 --targetname $TARGET --op new
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal 
$PORTAL:3260,1 --target $TARGET --op update --name node.session.auth.authmethod 
--value CHAP
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal 
$PORTAL:3260,1 --target $TARGET --op update --name node.session.auth.username 
--value $USER
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal 
$PORTAL:3260,1 --target $TARGET --op update --name node.session.auth.password 
--value $PASS
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode iface
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode iface 
--interface libvirt-iface-28727243 --op new
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode iface 
--interface libvirt-iface-28727243 --op update --name iface.initiatorname 
--value $INITIATOR
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode iface
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode discovery --type 
sendtargets --portal $PORTAL:3260,1 --interface libvirt-iface-28727243
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal 
$PORTAL:3260,1 --targetname $TARGET --login --interface libvirt-iface-28727243
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode session
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode session -r 1 -R


Thanks,
Michal

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

Re: [libvirt] [PATCH 3/5] virStorageBackendIQNFound: Rework iscsiadm output parsing

[Michal Prívozník]

On 07/03/2018 01:18 AM, John Ferlan wrote:
> 
> 
> On 06/29/2018 11:01 AM, Michal Privoznik wrote:
>> Firstly, we can utilize virCommandSetOutputBuffer() API which
>> will collect the command output for us. Secondly, sscanf()-ing
>> through each line is easier to understand (and more robust) than
>> jumping over a string with strchr().
>>
>> Signed-off-by: Michal Privoznik 
>> ---
>>  src/util/viriscsi.c | 85 
>> +
>>  1 file changed, 34 insertions(+), 51 deletions(-)
>>
> 
> I assume virCommandSetOutputBuffer didn't exist when this code was created.

Perhaps. Let me check git log. .. .. And yes, you are right.
virCommandSetOutputBuffer was introduced among with other basic
virCommand* APIs by Dan in late 2010 (f16ad06fb2aeb5e5c9974). But this
function was introduced by Dave in Jan 2010 so he couldn't have used it.

> 
> Also, why do I have this recollection related to portability of sscanf?
> I know we use it elsewhere, but some oddball issue that someone like
> Eric may recollect or know about.

I don't know about anything. But since we use it in our code pretty
freely I assumed there is no problem with it.

> 
>> diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c
>> index 2e55b3c10b..44788056fd 100644
>> --- a/src/util/viriscsi.c
>> +++ b/src/util/viriscsi.c
>> @@ -108,7 +108,6 @@ virISCSIGetSession(const char *devpath,
>>  
>>  
>>  
>> -#define LINE_SIZE 4096
>>  #define IQN_FOUND 1
>>  #define IQN_MISSING 0
>>  #define IQN_ERROR -1
>> @@ -117,71 +116,56 @@ static int
>>  virStorageBackendIQNFound(const char *initiatoriqn,
>>char **ifacename)
>>  {
>> -int ret = IQN_ERROR, fd = -1;
>> -char ebuf[64];
>> -FILE *fp = NULL;
>> -char *line = NULL, *newline = NULL, *iqn = NULL, *token = NULL;
>> +int ret = IQN_ERROR;
>> +char *outbuf = NULL;
>> +char *line = NULL;
>> +char *iface = NULL;
>> +char *iqn = NULL;
>>  virCommandPtr cmd = virCommandNewArgList(ISCSIADM,
>>   "--mode", "iface", NULL);
>>  
>>  *ifacename = NULL;
>>  
>> -if (VIR_ALLOC_N(line, LINE_SIZE) != 0) {
>> -virReportError(VIR_ERR_INTERNAL_ERROR,
>> -   _("Could not allocate memory for output of '%s'"),
>> -   ISCSIADM);
>> +virCommandSetOutputBuffer(cmd, );
>> +if (virCommandRun(cmd, NULL) < 0)
>>  goto cleanup;
>> -}
>>  
>> -memset(line, 0, LINE_SIZE);
>> +/* Example of data we are dealing with:
>> + * default tcp
>> + * iser iser
>> + * libvirt-iface-253db048 
>> tcpiqn.2017-03.com.user:client
>> + */
> 
> Nice to have an example especially since this is a bit of a edge case...
> 
> Another option - virStringSplitCount on the "\n" to get the number of
> lines and then on each line again using "," to tear apart the pieces.
> This I assume works as I don't have a initiatoriqn set up to test.
> 
> Any thoughts/recollections about sscanf? I assume it'll be felt that
> virStringSplit might be overkill, 

Indeed.

> but at least it's for other purposes
> and perhaps less susceptible to the line && *line adjustment.

I like sscanf() more because it's fewer lines of code, the variables I
need are assigned value immediately and also memory footprint is smaller
(I guess) since there's no need to store multiple arrays.

Michal

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

Re: [libvirt] [PATCH 4/5] virISCSIScanTargets: Honour iSCSI interface

[Michal Prívozník]

On 07/03/2018 01:38 AM, John Ferlan wrote:
> 
> 
> On 06/29/2018 11:01 AM, Michal Privoznik wrote:
>> When scanning for targets, iSCSI might give different results
>> depending on the interface used. This is basically just name of
> 
> assuming this means whether the initiatoriqn interface was used

Yes.

> 
>> config file under /etc/iscsi/ifaces to use. The file contains
>> initiator IQN thus different results claim.
>>
> 
> Strange to have activity here - was there a bz? Or something found by
> the (I assume) GSoC project:
> 
> https://www.redhat.com/archives/libvir-list/2018-June/msg00249.html
> 
> Touching something that's been avoided for 8 years is always scary, but
> if it's broken, then sure it ought to be fixed.

There is no BZ. And yes, the GSoC project got me experimenting (because
for libiscsi backend we will have to make IQN required since libiscsi
does not parse anything from /etc/iscsi nor initiatorname.iscsi). And
while experimenting I've tired to put my own IQN into iscsi pool we
already have and found this bug.

> 
>> Signed-off-by: Michal Privoznik 
>> ---
>>  src/storage/storage_backend_iscsi.c |  4 +-
>>  src/util/viriscsi.c | 78 
>> ++---
>>  src/util/viriscsi.h |  1 +
>>  tests/viriscsitest.c|  2 +-
>>  4 files changed, 77 insertions(+), 8 deletions(-)
>>
> 
> This patch fails to compile for me:
> 
> In file included from util/viriscsi.c:32:0:
> util/viriscsi.c: In function 'virISCSIScanTargets':
> util/virerror.h:187:5: error: this statement may fall through
> [-Werror=implicit-fallthrough=]
>  virReportErrorHelper(VIR_FROM_THIS, code, __FILE__, \
>  ^
>   __FUNCTION__, __LINE__, __VA_ARGS__)
>   
> util/viriscsi.c:479:13: note: in expansion of macro 'virReportError'
>  virReportError(VIR_ERR_OPERATION_FAILED,
>  ^~
> util/viriscsi.c:482:9: note: here
>  case IQN_ERROR:

Ah, okay. My gcc doesn't warn me about this. Wonder if I have something
misconfigured.

>  ^~~~
> 
>> diff --git a/src/storage/storage_backend_iscsi.c 
>> b/src/storage/storage_backend_iscsi.c
>> index 7871d1915b..3b9dddb4fd 100644
>> --- a/src/storage/storage_backend_iscsi.c
>> +++ b/src/storage/storage_backend_iscsi.c
>> @@ -194,7 +194,9 @@ virStorageBackendISCSIFindPoolSources(const char 
>> *srcSpec,
>>  if (!(portal = virStorageBackendISCSIPortal(source)))
>>  goto cleanup;
>>  
>> -if (virISCSIScanTargets(portal, , ) < 0)
>> +if (virISCSIScanTargets(portal,
>> +source->initiator.iqn,
> 
> NIT: This could go on the previous line
> 
>> +, ) < 0)
>>  goto cleanup;
>>  
>>  if (VIR_ALLOC_N(list.sources, ntargets) < 0)
>> diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c
>> index 44788056fd..365669aac2 100644
>> --- a/src/util/viriscsi.c
>> +++ b/src/util/viriscsi.c
>> @@ -40,6 +40,13 @@
>>  VIR_LOG_INIT("util.iscsi");
>>  
>>  
>> +static int
>> +virISCSIScanTargetsInternal(const char *portal,
>> +const char *ifacename,
>> +size_t *ntargetsret,
>> +char ***targetsret);
>> +
>> +
>>  struct virISCSISessionData {
>>  char *session;
>>  const char *devpath;
>> @@ -286,9 +293,10 @@ virISCSIConnection(const char *portal,
>>   * iscsiadm doesn't let you send commands to the Interface IQN,
>>   * unless you've first issued a 'sendtargets' command to the
>>   * portal. Without the sendtargets all that is received is a
>> - * "iscsiadm: No records found"
>> + * "iscsiadm: No records found". However, we must ensure that
>> + * the command is issued over interface name we invented above.
> 
> "invented" - again is this the make sure it's issued over the
> initiatoriqn interface?

Yes. And we construct the interface name at the beginning of this function.

Michal

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

Re: [libvirt] [RFCv2 PATCH 3/5] conf: rename cachetune to restune

[bing.niu]

Hi John,
Thanks for reviewing! Since major questions are from this thread, so I 
think we can start from this.


On 2018年06月30日 06:47, John Ferlan wrote:



On 06/15/2018 05:29 AM, bing@intel.com wrote:

From: Bing Niu 

resctrl not only supports cache tuning, but also memory bandwidth
tuning. Rename cachetune to restune(resource tuning) to reflect
that.

Signed-off-by: Bing Niu 
---
  src/conf/domain_conf.c  | 44 ++--
  src/conf/domain_conf.h  | 10 +-
  src/qemu/qemu_process.c | 18 +-
  3 files changed, 36 insertions(+), 36 deletions(-)



Not so sure I'm liking [R|r]estune[s]... Still @cachetunes is an array
of a structure that contains a vCPU bitmap and a virResctrlAllocPtr for
the /cputune/cachetunes data.  The virResctrlAllocPtr was changed to add
a the bandwidth allocation, so does this really have to change?

The cachetunes from Cache Allocation Technology(CAT) and memorytunes 
from Memory Bandwidth Allocation(MBA) are both features from Resource 
Directory Technology. RDT is a collection of resource distribution 
technology, right now it includes CAT and MBA. Details information can 
be found 17.18.1 of Intel's sdm.

https://software.intel.com/sites/default/files/managed/39/c5/325462-sdm-vol-1-2abcd-3abcd.pdf

The RDT capability and configuration methods is exposed to user land in 
form of resctrl file system by kernel. The basic programming model for 
this resctrl fs interface like this:


1. create a folder under /sys/fs/resctrl. And this folder is called one 
resctrl group.


2. writing user desired CAT and MBA config to the file 
/sys/fs/resctrl//schemata to allocate the cache or 
memory bandwidth. you can set CAT and MBA together or any of them.


3. moving the threads you want to that resctrl group by writing threads' 
PID to /sys/fs/resctrl//task file. So that those 
threads can be assign with the resource allocated in step 2. And those 
resources are shared by those threads.


*NOTE*: A thread only can be put in one resctrl group. This requirement 
is from how RDT and resctrl works.
Each resctrl group has a closid. The resource configuration in above 
step2 is set to that closid's msr(IA32_L?_QoS) to describe the resource 
allocation for this closid.
And thread use this closid to claim the allocated resource during 
context switch(scheduled_in()) in kernel by writing the closid to the 
msr IA32_PQR_ASSOC.
With closid wrote in IA32_PQR_ASSOC, RDT hardware knows current running 
closid and allocated resource basing on this closid's IA32_L?_QoS msr.

That why a thread can be put into one restrcl group only.

Details description of resctrl can be found:
https://github.com/torvalds/linux/blob/v4.17/Documentation/x86/intel_rdt_ui.txt

:)

Basing on above information, my understanding is that virResctrlAllocPtr 
is not only bind to cachetunes. It should be a backing object to 
describe a resctrl group. Especially current virresctrl class already 
works as that.
Libvirt will create one resctrl group for each virResctrlAllocPtr by 
virResctrlAllocCreate() interface.


So from components view, the big picture is something like below.


   
   +-+
 |
   XML   |   +
  parser +---+
 |
 |
+--+
 |
 |
internal resctrl  +--v--+
backing object|  virResctrlAllocPtr |
  +--+--+
 |
 |
+--+
 |
  +--v---+
  |  |
  | schemata |
 /sys/fs/resctrl  | tasks|
  |   .  |
  |   .  |
  |  |
  +--+

Does this make sense? :)


I think the question is - if both cachetunes and memtunes exist in
domain XML, then for which does the @vcpus relate to. That is, from the
cover there's:

 
   
 

and then there's a

 
   
   
 

Now what?  I haven't looked at patch 4 yet to even know how this "works".


I also raised this concern in the first round review. Ideally, if we can 
have a domain XML like this



 

 
  


  


That will be more clear. Above domain XML means: one resctrl group with 
memory bandwidth 20 @node 0, L3 cache 3MB @ node0 and node 1. Put vcpu 
'0-3' to this resctrl group. So those resources are shared among vcpus.
However, Pavel pointed out that we must keep backward compatible. And we 
need keep .


In RFC v1, I also proposed to put memory bandwidth into cachetunes 
section like:

  


  

[libvirt] [PATCH 2/2] storage: Improve error handling on cdrom backend

[Han Han]

Implement virFileCdromStatus() in virStorageBackendVolOpen to show
detailed errors or warnings of cdrom instead of general Input/output error.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1596096
Signed-off-by: Han Han 
---
 src/storage/storage_util.c | 38 ++
 1 file changed, 38 insertions(+)

diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c
index a701a75702..5a7ed4c76f 100644
--- a/src/storage/storage_util.c
+++ b/src/storage/storage_util.c
@@ -1538,6 +1538,44 @@ virStorageBackendVolOpen(const char *path, struct stat 
*sb,
 return -1;
 }
 
+if (virFileIsCDROM(path) == 1) {
+switch (virFileCdromStatus(path)) {
+case VIR_FILE_CDROM_NO_INFO:
+if (noerror) {
+VIR_WARN("ignoring unavailable information of %s", path);
+return -2;
+}
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Cdrom %s information is unavailable"), path);
+return -1;
+case VIR_FILE_CDROM_NO_DISC :
+if (noerror) {
+VIR_WARN("ignoring no disc %s", path);
+return -2;
+}
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Cdrom %s has no disc"), path);
+return -1;
+case VIR_FILE_CDROM_TREY_OPEN :
+if (noerror) {
+VIR_WARN("ignoring trey open of %s", path);
+return -2;
+}
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Cdrom %s is on trey-open status"), path);
+return -1;
+case VIR_FILE_CDROM_DRIVE_NOT_READY :
+if (noerror) {
+VIR_WARN("ignoring %s not ready", path);
+return -2;
+}
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Cdrom %s is on not ready"), path);
+return -1;
+case VIR_FILE_CDROM_DISC_OK :
+VIR_INFO("Cdrom %s status is ok", path);
+}
+}
 /* O_NONBLOCK should only matter during open() for fifos and
  * sockets, which we already filtered; but using it prevents a
  * TOCTTOU race.  However, later on we will want to read() the
-- 
2.17.1

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

[libvirt] [PATCH 0/2] Add function to check cdrom status

[Han Han]

In libvirt, it gives general Input/Output error when reading a not ready
cdrom. This error could be more detailed on cdrom, such as drive not
ready, no disc, trey open, no information.
These patches provide private API virFileCdromStatus to check cdrom storage 
backend
status and improve error handling of virStorageBackendVolOpen on cdrom.

Han Han (2):
  util: Introduce virFileCdromStatus
  storage: Improve error handling on cdrom backend

 src/libvirt_private.syms   |  1 +
 src/storage/storage_util.c | 38 
 src/util/virfile.c | 52 ++
 src/util/virfile.h | 10 
 4 files changed, 101 insertions(+)

-- 
2.17.1

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

[libvirt] [PATCH 1/2] util: Introduce virFileCdromStatus

[Han Han]

This private API helps check cdrom drive status via ioctl().

Signed-off-by: Han Han 
---
 src/libvirt_private.syms |  1 +
 src/util/virfile.c   | 52 
 src/util/virfile.h   | 10 
 3 files changed, 63 insertions(+)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 5499a368c0..e9f79ad433 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1797,6 +1797,7 @@ virFileActivateDirOverride;
 virFileBindMountDevice;
 virFileBuildPath;
 virFileCanonicalizePath;
+virFileCdromStatus;
 virFileChownFiles;
 virFileClose;
 virFileComparePaths;
diff --git a/src/util/virfile.c b/src/util/virfile.c
index 378d03ecf0..790d9448d2 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -2026,6 +2026,58 @@ virFileIsCDROM(const char *path)
 return ret;
 }
 
+/**
+ * virFileCdromStatus:
+ * @path: Cdrom path
+ *
+ * Returns:
+ *  -1  error happends.
+ *  VIR_FILE_CDROM_DISC_OK  Cdrom is OK.
+ *  VIR_FILE_CDROM_NO_INFO  Information not available.
+ *  VIR_FILE_CDROM_NO_DISC  No disc in cdrom.
+ *  VIR_FILE_CDROM_TREY_OPENCdrom is trey-open.
+ *  VIR_FILE_CDROM_DRIVE_NOT_READY  Cdrom is not ready.
+ * reported.
+ */
+int
+virFileCdromStatus(const char *path)
+{
+int ret = -1;
+int fd;
+
+if ((fd = open(path, O_RDONLY | O_NONBLOCK)) < 0)
+goto cleanup;
+
+/* Attempt to detect CDROM status via ioctl */
+if ((ret = ioctl(fd, CDROM_DRIVE_STATUS, CDSL_CURRENT)) >= 0) {
+switch (ret) {
+case CDS_NO_INFO:
+ret = VIR_FILE_CDROM_NO_INFO;
+goto cleanup;
+break;
+case CDS_NO_DISC:
+ret = VIR_FILE_CDROM_NO_DISC;
+goto cleanup;
+break;
+case CDS_TRAY_OPEN:
+ret = VIR_FILE_CDROM_TREY_OPEN;
+goto cleanup;
+break;
+case CDS_DRIVE_NOT_READY:
+ret = VIR_FILE_CDROM_DRIVE_NOT_READY;
+goto cleanup;
+break;
+case CDS_DISC_OK:
+ret = VIR_FILE_CDROM_DISC_OK;
+goto cleanup;
+break;
+}
+}
+
+ cleanup:
+VIR_FORCE_CLOSE(fd);
+return ret;
+}
 #else
 
 int
diff --git a/src/util/virfile.h b/src/util/virfile.h
index 6f1e802fde..9d4701c8d2 100644
--- a/src/util/virfile.h
+++ b/src/util/virfile.h
@@ -212,6 +212,16 @@ int virFileIsSharedFS(const char *path) 
ATTRIBUTE_NONNULL(1);
 int virFileIsMountPoint(const char *file) ATTRIBUTE_NONNULL(1);
 int virFileIsCDROM(const char *path)
 ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
+int virFileCdromStatus(const char *path)
+ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
+
+enum {
+VIR_FILE_CDROM_DISC_OK = 1,
+VIR_FILE_CDROM_NO_INFO,
+VIR_FILE_CDROM_NO_DISC,
+VIR_FILE_CDROM_TREY_OPEN,
+VIR_FILE_CDROM_DRIVE_NOT_READY,
+};
 
 int virFileGetMountSubtree(const char *mtabpath,
const char *prefix,
-- 
2.17.1

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

[libvirt] [PATCH 3/3] esx_driver: Use virCheckFlag macro

[Marcos Paulo de Souza]

Instead of duplicating code to do the same checking. Now all functions
of virHypervisorDriver from esx driver are using this macro.

Signed-off-by: Marcos Paulo de Souza 
---
 src/esx/esx_driver.c | 10 ++
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c
index 60aa5fc252..705b0d1a41 100644
--- a/src/esx/esx_driver.c
+++ b/src/esx/esx_driver.c
@@ -2488,10 +2488,7 @@ esxDomainSetVcpusFlags(virDomainPtr domain, unsigned int 
nvcpus,
 esxVI_TaskInfoState taskInfoState;
 char *taskInfoErrorMessage = NULL;
 
-if (flags != VIR_DOMAIN_AFFECT_LIVE) {
-virReportError(VIR_ERR_INVALID_ARG, _("unsupported flags: (0x%x)"), 
flags);
-return -1;
-}
+virCheckFlags(VIR_DOMAIN_AFFECT_LIVE, -1);
 
 if (nvcpus < 1) {
 virReportError(VIR_ERR_INVALID_ARG, "%s",
@@ -2570,10 +2567,7 @@ esxDomainGetVcpusFlags(virDomainPtr domain, unsigned int 
flags)
 esxVI_ObjectContent *hostSystem = NULL;
 esxVI_DynamicProperty *dynamicProperty = NULL;
 
-if (flags != (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_VCPU_MAXIMUM)) {
-virReportError(VIR_ERR_INVALID_ARG, _("unsupported flags: (0x%x)"), 
flags);
-return -1;
-}
+virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_VCPU_MAXIMUM, -1);
 
 if (priv->maxVcpus > 0)
 return priv->maxVcpus;
-- 
2.17.1

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

[libvirt] [PATCH 1/3] esx_vi.c: Simplify error handling in MachineByName

[Marcos Paulo de Souza]

The same pattern is used in lots of other places.

Signed-off-by: Marcos Paulo de Souza 
---
 src/esx/esx_vi.c | 14 --
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c
index 43ff7ea048..25fbdc7e44 100644
--- a/src/esx/esx_vi.c
+++ b/src/esx/esx_vi.c
@@ -3014,16 +3014,10 @@ esxVI_LookupVirtualMachineByName(esxVI_Context *ctx, 
const char *name,
 break;
 }
 
-if (!(*virtualMachine)) {
-if (occurrence == esxVI_Occurrence_OptionalItem) {
-result = 0;
-
-goto cleanup;
-} else {
-virReportError(VIR_ERR_NO_DOMAIN,
-   _("Could not find domain with name '%s'"), name);
-goto cleanup;
-}
+if (!(*virtualMachine) && occurrence != esxVI_Occurrence_OptionalItem) {
+virReportError(VIR_ERR_NO_DOMAIN,
+   _("Could not find domain with name '%s'"), name);
+goto cleanup;
 }
 
 result = 0;
-- 
2.17.1

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

[libvirt] [PATCH 2/3] esx: Add esxVI_checkArgList macro

[Marcos Paulo de Souza]

This macro avoids code duplication when checking for arrays of objects.

Signed-off-by: Marcos Paulo de Souza 
---
 src/esx/esx_vi.c | 189 ---
 1 file changed, 46 insertions(+), 143 deletions(-)

diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c
index 25fbdc7e44..212300dbff 100644
--- a/src/esx/esx_vi.c
+++ b/src/esx/esx_vi.c
@@ -41,6 +41,16 @@
 
 VIR_LOG_INIT("esx.esx_vi");
 
+#define esxVI_checkArgList(val) \
+do { \
+if (!val || *val) { \
+virReportError(VIR_ERR_INVALID_ARG, "%s", _("Invalid argument")); \
+return -1; \
+} \
+} while (0)
+
+
+
 #define ESX_VI__SOAP__RESPONSE_XPATH(_type) \
 ((char *)"/soapenv:Envelope/soapenv:Body/" \
"vim:"_type"Response/vim:returnval")
@@ -51,11 +61,7 @@ VIR_LOG_INIT("esx.esx_vi");
 int \
 esxVI_##_type##_Alloc(esxVI_##_type **ptrptr) \
 { \
-if (!ptrptr || *ptrptr) { \
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid 
argument")); \
-return -1; \
-} \
- \
+esxVI_checkArgList(ptrptr); \
 if (VIR_ALLOC(*ptrptr) < 0) \
 return -1; \
 return 0; \
@@ -372,10 +378,7 @@ esxVI_CURL_Download(esxVI_CURL *curl, const char *url, 
char **content,
 virBuffer buffer = VIR_BUFFER_INITIALIZER;
 int responseCode = 0;
 
-if (!content || *content) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument"));
-return -1;
-}
+esxVI_checkArgList(content);
 
 if (length && *length > 0) {
 /*
@@ -1762,10 +1765,7 @@ esxVI_List_DeepCopy(esxVI_List **destList, esxVI_List 
*srcList,
 esxVI_List *dest = NULL;
 esxVI_List *src = NULL;
 
-if (!destList || *destList) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument"));
-return -1;
-}
+esxVI_checkArgList(destList);
 
 for (src = srcList; src; src = src->_next) {
 if (deepCopyFunc(, src) < 0 ||
@@ -2170,10 +2170,7 @@ esxVI_LookupObjectContentByType(esxVI_Context *ctx,
 bool propertySpec_isAppended = false;
 esxVI_PropertyFilterSpec *propertyFilterSpec = NULL;
 
-if (!objectContentList || *objectContentList) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument"));
-return -1;
-}
+esxVI_checkArgList(objectContentList);
 
 if (esxVI_ObjectSpec_Alloc() < 0)
 return -1;
@@ -2372,10 +2369,7 @@ esxVI_GetVirtualMachineQuestionInfo
 {
 esxVI_DynamicProperty *dynamicProperty;
 
-if (!questionInfo || *questionInfo) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument"));
-return -1;
-}
+esxVI_checkArgList(questionInfo);
 
 for (dynamicProperty = virtualMachine->propSet; dynamicProperty;
  dynamicProperty = dynamicProperty->_next) {
@@ -2447,10 +2441,7 @@ esxVI_GetInt(esxVI_ObjectContent *objectContent, const 
char *propertyName,
 {
 esxVI_DynamicProperty *dynamicProperty;
 
-if (!value || *value) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument"));
-return -1;
-}
+esxVI_checkArgList(value);
 
 for (dynamicProperty = objectContent->propSet; dynamicProperty;
  dynamicProperty = dynamicProperty->_next) {
@@ -2479,10 +2470,7 @@ esxVI_GetLong(esxVI_ObjectContent *objectContent, const 
char *propertyName,
 {
 esxVI_DynamicProperty *dynamicProperty;
 
-if (!value || *value) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument"));
-return -1;
-}
+esxVI_checkArgList(value);
 
 for (dynamicProperty = objectContent->propSet; dynamicProperty;
  dynamicProperty = dynamicProperty->_next) {
@@ -2512,10 +2500,7 @@ esxVI_GetStringValue(esxVI_ObjectContent *objectContent,
 {
 esxVI_DynamicProperty *dynamicProperty;
 
-if (!value || *value) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument"));
-return -1;
-}
+esxVI_checkArgList(value);
 
 for (dynamicProperty = objectContent->propSet; dynamicProperty;
  dynamicProperty = dynamicProperty->_next) {
@@ -2549,10 +2534,7 @@ esxVI_GetManagedObjectReference(esxVI_ObjectContent 
*objectContent,
 {
 esxVI_DynamicProperty *dynamicProperty;
 
-if (!value || *value) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument"));
-return -1;
-}
+esxVI_checkArgList(value);
 
 for (dynamicProperty = objectContent->propSet; dynamicProperty;
  dynamicProperty = dynamicProperty->_next) {
@@ -2863,10 +2845,7 @@ esxVI_GetSnapshotTreeBySnapshot
 {
 esxVI_VirtualMachineSnapshotTree *candidate;
 
-if (!snapshotTree || *snapshotTree) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument"));
-return -1;
-}
+esxVI_checkArgList(snapshotTree);
 
 for (candidate = snapshotTreeList; candidate;
  candidate = candidate->_next) 

Re: [libvirt] [PATCH 5/5] virISCSIScanTargets: Allow making targets persistent

[John Ferlan]

On 06/29/2018 11:01 AM, Michal Privoznik wrote:
> After new iSCSI interface is successfully set up, we issue

s/new/a new/
s/issue/issue a/

> sendtargets command. However, after 56057900dc53df490d we don't
> update the host config which in turn makes login fail because
> iscsiadm is unable to find any matching record for the interface.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/storage/storage_backend_iscsi.c |  1 +
>  src/util/viriscsi.c | 21 ++---
>  src/util/viriscsi.h |  1 +
>  tests/viriscsitest.c|  3 ++-
>  4 files changed, 22 insertions(+), 4 deletions(-)
> 

Like the previous patch - is there a specific bug or something that led
you down this path?  Can you show an example of the existing code that's
creating a bad command line and generating an error and then how this
fixes things.  It's not like we have tests and for this stuff it's
really nice to have plenty of examples.

> diff --git a/src/storage/storage_backend_iscsi.c 
> b/src/storage/storage_backend_iscsi.c
> index 3b9dddb4fd..6242cd0fac 100644
> --- a/src/storage/storage_backend_iscsi.c
> +++ b/src/storage/storage_backend_iscsi.c
> @@ -196,6 +196,7 @@ virStorageBackendISCSIFindPoolSources(const char *srcSpec,
>  
>  if (virISCSIScanTargets(portal,
>  source->initiator.iqn,
> +false,
>  , ) < 0)
>  goto cleanup;
>  
> diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c
> index 365669aac2..baf41c5be1 100644
> --- a/src/util/viriscsi.c
> +++ b/src/util/viriscsi.c
> @@ -43,6 +43,7 @@ VIR_LOG_INIT("util.iscsi");
>  static int
>  virISCSIScanTargetsInternal(const char *portal,
>  const char *ifacename,
> +bool persist,
>  size_t *ntargetsret,
>  char ***targetsret);
>  
> @@ -295,8 +296,10 @@ virISCSIConnection(const char *portal,
>   * portal. Without the sendtargets all that is received is a
>   * "iscsiadm: No records found". However, we must ensure that
>   * the command is issued over interface name we invented above.

s/above./above/

> + * AND that targets are made persistent.

s/AND/and/

Similar to previous - with minor adjustments and explanation,

Reviewed-by: John Ferlan 

John

>   */
> -if (virISCSIScanTargetsInternal(portal, ifacename, NULL, NULL) < 
> 0)
> +if (virISCSIScanTargetsInternal(portal, ifacename,
> +true, NULL, NULL) < 0)
>  goto cleanup;
>  
>  break;
[...]

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

Re: [libvirt] [PATCH 4/5] virISCSIScanTargets: Honour iSCSI interface

[John Ferlan]

On 06/29/2018 11:01 AM, Michal Privoznik wrote:
> When scanning for targets, iSCSI might give different results
> depending on the interface used. This is basically just name of

assuming this means whether the initiatoriqn interface was used

> config file under /etc/iscsi/ifaces to use. The file contains
> initiator IQN thus different results claim.
> 

Strange to have activity here - was there a bz? Or something found by
the (I assume) GSoC project:

https://www.redhat.com/archives/libvir-list/2018-June/msg00249.html

Touching something that's been avoided for 8 years is always scary, but
if it's broken, then sure it ought to be fixed.

> Signed-off-by: Michal Privoznik 
> ---
>  src/storage/storage_backend_iscsi.c |  4 +-
>  src/util/viriscsi.c | 78 
> ++---
>  src/util/viriscsi.h |  1 +
>  tests/viriscsitest.c|  2 +-
>  4 files changed, 77 insertions(+), 8 deletions(-)
> 

This patch fails to compile for me:

In file included from util/viriscsi.c:32:0:
util/viriscsi.c: In function 'virISCSIScanTargets':
util/virerror.h:187:5: error: this statement may fall through
[-Werror=implicit-fallthrough=]
 virReportErrorHelper(VIR_FROM_THIS, code, __FILE__, \
 ^
  __FUNCTION__, __LINE__, __VA_ARGS__)
  
util/viriscsi.c:479:13: note: in expansion of macro 'virReportError'
 virReportError(VIR_ERR_OPERATION_FAILED,
 ^~
util/viriscsi.c:482:9: note: here
 case IQN_ERROR:
 ^~~~

> diff --git a/src/storage/storage_backend_iscsi.c 
> b/src/storage/storage_backend_iscsi.c
> index 7871d1915b..3b9dddb4fd 100644
> --- a/src/storage/storage_backend_iscsi.c
> +++ b/src/storage/storage_backend_iscsi.c
> @@ -194,7 +194,9 @@ virStorageBackendISCSIFindPoolSources(const char *srcSpec,
>  if (!(portal = virStorageBackendISCSIPortal(source)))
>  goto cleanup;
>  
> -if (virISCSIScanTargets(portal, , ) < 0)
> +if (virISCSIScanTargets(portal,
> +source->initiator.iqn,

NIT: This could go on the previous line

> +, ) < 0)
>  goto cleanup;
>  
>  if (VIR_ALLOC_N(list.sources, ntargets) < 0)
> diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c
> index 44788056fd..365669aac2 100644
> --- a/src/util/viriscsi.c
> +++ b/src/util/viriscsi.c
> @@ -40,6 +40,13 @@
>  VIR_LOG_INIT("util.iscsi");
>  
>  
> +static int
> +virISCSIScanTargetsInternal(const char *portal,
> +const char *ifacename,
> +size_t *ntargetsret,
> +char ***targetsret);
> +
> +
>  struct virISCSISessionData {
>  char *session;
>  const char *devpath;
> @@ -286,9 +293,10 @@ virISCSIConnection(const char *portal,
>   * iscsiadm doesn't let you send commands to the Interface IQN,
>   * unless you've first issued a 'sendtargets' command to the
>   * portal. Without the sendtargets all that is received is a
> - * "iscsiadm: No records found"
> + * "iscsiadm: No records found". However, we must ensure that
> + * the command is issued over interface name we invented above.

"invented" - again is this the make sure it's issued over the
initiatoriqn interface?

>   */
> -if (virISCSIScanTargets(portal, NULL, NULL) < 0)
> +if (virISCSIScanTargetsInternal(portal, ifacename, NULL, NULL) < 
> 0)
>  goto cleanup;
>  
>  break;
> @@ -371,10 +379,11 @@ virISCSIGetTargets(char **const groups,
>  }
>  
>  
> -int
> -virISCSIScanTargets(const char *portal,
> -size_t *ntargetsret,
> -char ***targetsret)
> +static int
> +virISCSIScanTargetsInternal(const char *portal,
> +const char *ifacename,
> +size_t *ntargetsret,
> +char ***targetsret)
>  {
>  /**
>   *
> @@ -400,6 +409,12 @@ virISCSIScanTargets(const char *portal,
>   "--op", "nonpersistent",
>   NULL);
>  
> +if (ifacename) {
> +virCommandAddArgList(cmd,
> + "--interface", ifacename,
> + NULL);
> +}
> +

Could just be one line to avoid the { }

>  memset(, 0, sizeof(list));
>  
>  if (virCommandRunRegex(cmd,
> @@ -425,6 +440,57 @@ virISCSIScanTargets(const char *portal,
>  return ret;
>  }
>  
> +
> +/**
> + * virISCSIScanTargets:
> + * @portal: iSCSI portal
> + * @initiatoriqn: Initiator IQN
> + * @ntargets: number of items in @targetsret array
> + * @targets: array of targets
> + *
> + * For given @portal issue sendtargets command. Optionally,
> + * 

Re: [libvirt] [PATCH 3/5] virStorageBackendIQNFound: Rework iscsiadm output parsing

[John Ferlan]

On 06/29/2018 11:01 AM, Michal Privoznik wrote:
> Firstly, we can utilize virCommandSetOutputBuffer() API which
> will collect the command output for us. Secondly, sscanf()-ing
> through each line is easier to understand (and more robust) than
> jumping over a string with strchr().
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/util/viriscsi.c | 85 
> +
>  1 file changed, 34 insertions(+), 51 deletions(-)
> 

I assume virCommandSetOutputBuffer didn't exist when this code was created.

Also, why do I have this recollection related to portability of sscanf?
I know we use it elsewhere, but some oddball issue that someone like
Eric may recollect or know about.

> diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c
> index 2e55b3c10b..44788056fd 100644
> --- a/src/util/viriscsi.c
> +++ b/src/util/viriscsi.c
> @@ -108,7 +108,6 @@ virISCSIGetSession(const char *devpath,
>  
>  
>  
> -#define LINE_SIZE 4096
>  #define IQN_FOUND 1
>  #define IQN_MISSING 0
>  #define IQN_ERROR -1
> @@ -117,71 +116,56 @@ static int
>  virStorageBackendIQNFound(const char *initiatoriqn,
>char **ifacename)
>  {
> -int ret = IQN_ERROR, fd = -1;
> -char ebuf[64];
> -FILE *fp = NULL;
> -char *line = NULL, *newline = NULL, *iqn = NULL, *token = NULL;
> +int ret = IQN_ERROR;
> +char *outbuf = NULL;
> +char *line = NULL;
> +char *iface = NULL;
> +char *iqn = NULL;
>  virCommandPtr cmd = virCommandNewArgList(ISCSIADM,
>   "--mode", "iface", NULL);
>  
>  *ifacename = NULL;
>  
> -if (VIR_ALLOC_N(line, LINE_SIZE) != 0) {
> -virReportError(VIR_ERR_INTERNAL_ERROR,
> -   _("Could not allocate memory for output of '%s'"),
> -   ISCSIADM);
> +virCommandSetOutputBuffer(cmd, );
> +if (virCommandRun(cmd, NULL) < 0)
>  goto cleanup;
> -}
>  
> -memset(line, 0, LINE_SIZE);
> +/* Example of data we are dealing with:
> + * default tcp
> + * iser iser
> + * libvirt-iface-253db048 
> tcpiqn.2017-03.com.user:client
> + */

Nice to have an example especially since this is a bit of a edge case...

Another option - virStringSplitCount on the "\n" to get the number of
lines and then on each line again using "," to tear apart the pieces.
This I assume works as I don't have a initiatoriqn set up to test.

Any thoughts/recollections about sscanf? I assume it'll be felt that
virStringSplit might be overkill, but at least it's for other purposes
and perhaps less susceptible to the line && *line adjustment.

IDC - let's see if sscanf causes any other recollections before R-by'ing.

John

>  
> -virCommandSetOutputFD(cmd, );
> -if (virCommandRunAsync(cmd, NULL) < 0)
> -goto cleanup;
> +line = outbuf;
> +while (line) {
> +char *newline;
> +int num;
>  
> -if ((fp = VIR_FDOPEN(fd, "r")) == NULL) {
> -virReportError(VIR_ERR_INTERNAL_ERROR,
> -   _("Failed to open stream for file descriptor "
> - "when reading output from '%s': '%s'"),
> -   ISCSIADM, virStrerror(errno, ebuf, sizeof(ebuf)));
> -goto cleanup;
> -}
> +if (!(newline = strchr(line, '\n')))
> +break;
>  
> -while (fgets(line, LINE_SIZE, fp) != NULL) {
> -newline = strrchr(line, '\n');
> -if (newline == NULL) {
> -virReportError(VIR_ERR_INTERNAL_ERROR,
> -   _("Unexpected line > %d characters "
> - "when parsing output of '%s'"),
> -   LINE_SIZE, ISCSIADM);
> -goto cleanup;
> -}
>  *newline = '\0';
>  
> -iqn = strrchr(line, ',');
> -if (iqn == NULL)
> -continue;
> -iqn++;
> +VIR_FREE(iface);
> +VIR_FREE(iqn);
> +num = sscanf(line, "%ms %*[^,],%*[^,],%*[^,],%*[^,],%ms", , 
> );
> +
> +if (num != 2) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("malformed output of %s: %s"),
> +   ISCSIADM, line);
> +goto cleanup;
> +}
>  
>  if (STREQ(iqn, initiatoriqn)) {
> -token = strchr(line, ' ');
> -if (!token) {
> -virReportError(VIR_ERR_INTERNAL_ERROR,
> -   _("Missing space when parsing output "
> - "of '%s'"), ISCSIADM);
> -goto cleanup;
> -}
> -
> -if (VIR_STRNDUP(*ifacename, line, token - line) < 0)
> -goto cleanup;
> +VIR_STEAL_PTR(*ifacename, iface);
>  
>  VIR_DEBUG("Found interface '%s' with IQN '%s'", *ifacename, iqn);
>  break;
>  }
> -}
>  
> -if (virCommandWait(cmd, NULL) < 0)
> -  

Re: [libvirt] [PATCH 2/5] virStorageBackendIQNFound: Rename out label

[John Ferlan]

On 06/29/2018 11:01 AM, Michal Privoznik wrote:
> This is in fact 'cleanup' label and it should be named as such.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/util/viriscsi.c | 16 
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH 1/5] virStorageBackendIQNFound: Fix ret value assignment

[John Ferlan]

On 06/29/2018 11:01 AM, Michal Privoznik wrote:
> The return value of this function is overwritten more times than
> I am comfortable with.

True, but let's actually say what you're doing...

Perform some method clean-up to follow more accepted coding standards:

 * Initialize @ret to error value and prove otherwise.
 * Initialize *ifacename to NULL

> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/util/viriscsi.c | 25 +++--
>  1 file changed, 11 insertions(+), 14 deletions(-)
> 

Touching Dave's 8+ year old code (commit id 6aabcb5), lucky you.  I
believe this "feature" was never documented too - now that your name is
on top you can own it :-P!

> diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c
> index d4c745a1af..a308ee4bac 100644
> --- a/src/util/viriscsi.c
> +++ b/src/util/viriscsi.c
> @@ -117,15 +117,16 @@ static int
>  virStorageBackendIQNFound(const char *initiatoriqn,
>char **ifacename)
>  {
> -int ret = IQN_MISSING, fd = -1;
> +int ret = IQN_ERROR, fd = -1;
>  char ebuf[64];
>  FILE *fp = NULL;
>  char *line = NULL, *newline = NULL, *iqn = NULL, *token = NULL;
>  virCommandPtr cmd = virCommandNewArgList(ISCSIADM,
>   "--mode", "iface", NULL);
>  
> +*ifacename = NULL;
> +
>  if (VIR_ALLOC_N(line, LINE_SIZE) != 0) {
> -ret = IQN_ERROR;
>  virReportError(VIR_ERR_INTERNAL_ERROR,
> _("Could not allocate memory for output of '%s'"),
> ISCSIADM);
> @@ -135,24 +136,20 @@ virStorageBackendIQNFound(const char *initiatoriqn,
>  memset(line, 0, LINE_SIZE);
>  
>  virCommandSetOutputFD(cmd, );
> -if (virCommandRunAsync(cmd, NULL) < 0) {
> -ret = IQN_ERROR;
> +if (virCommandRunAsync(cmd, NULL) < 0)
>  goto out;
> -}
>  
>  if ((fp = VIR_FDOPEN(fd, "r")) == NULL) {
>  virReportError(VIR_ERR_INTERNAL_ERROR,
> _("Failed to open stream for file descriptor "
>   "when reading output from '%s': '%s'"),
> ISCSIADM, virStrerror(errno, ebuf, sizeof(ebuf)));
> -ret = IQN_ERROR;
>  goto out;
>  }
>  
>  while (fgets(line, LINE_SIZE, fp) != NULL) {
>  newline = strrchr(line, '\n');
>  if (newline == NULL) {
> -ret = IQN_ERROR;
>  virReportError(VIR_ERR_INTERNAL_ERROR,
> _("Unexpected line > %d characters "
>   "when parsing output of '%s'"),
> @@ -169,27 +166,27 @@ virStorageBackendIQNFound(const char *initiatoriqn,
>  if (STREQ(iqn, initiatoriqn)) {
>  token = strchr(line, ' ');
>  if (!token) {
> -ret = IQN_ERROR;
>  virReportError(VIR_ERR_INTERNAL_ERROR,
> _("Missing space when parsing output "
>   "of '%s'"), ISCSIADM);
>  goto out;
>  }
> -if (VIR_STRNDUP(*ifacename, line, token - line) < 0) {
> -ret = IQN_ERROR;
> +
> +if (VIR_STRNDUP(*ifacename, line, token - line) < 0)
>  goto out;
> -}
> +
>  VIR_DEBUG("Found interface '%s' with IQN '%s'", *ifacename, iqn);
> -ret = IQN_FOUND;
>  break;
>  }
>  }
>  
>  if (virCommandWait(cmd, NULL) < 0)
> -ret = IQN_ERROR;
> +goto out;
> +
> +ret = *ifacename ? IQN_FOUND : IQN_MISSING;
>  
>   out:

While you're at it s/out/cleanup

> -if (ret == IQN_MISSING)
> +if (ret != IQN_FOUND)

IDC since it's VIR_DEBUG, but you'll get this message on IQN_ERROR too
as well as the virReportError... I would think that's fairly obvious on
the error paths

With some minor cleanups...

Reviewed-by: John Ferlan 

John

>  VIR_DEBUG("Could not find interface with IQN '%s'", iqn);
>  
>  VIR_FREE(line);
> 

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

Re: [libvirt] [PATCH 1/3] util: new function virNetDevOpenvswitchInterfaceGetMaster()

[Laine Stump]

On 07/02/2018 02:11 AM, Michal Prívozník wrote:
> On 07/02/2018 01:46 AM, Laine Stump wrote:
>> +virNetDevOpenvswitchAddTimeout(cmd);
>> +virCommandAddArgList(cmd, "iface-to-br", ifname, NULL);
>> +virCommandSetOutputBuffer(cmd, master);
>> +
>> +if (virCommandRun(cmd, ) < 0) {
>> +virReportError(VIR_ERR_INTERNAL_ERROR,
>> +   _("Unable to run command to get OVS master for "
>> + "interface %s"), ifname);
>> +goto cleanup;
>> +}
>> +
>> +/* non-0 exit code just means that the interface has no master in OVS */
>> +if (exitstatus != 0)
>> +VIR_FREE(*master);
> A-ha! We indeed need to do this. I was under impression that just like
> other functions of ours if they fail no allocation is done. But this is
> different so we need to call VIR_FREE().

Right. If ovs-vsctl is run and returns non-0, there still might have
been some sort of output to stdout (or there might not have been, but I
don't want to rely on it).

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

Re: [libvirt] [PATCH 0/2] domain_addr: delete unused functions

[John Ferlan]

On 06/29/2018 11:03 AM, Anya Harter wrote:
> each commit references the commit where the last use was removed
> 
> Anya Harter (2):
>   domain_addr: delete virDomainCCWAddressReleaseAddr
>   domain_addr: delete virDomainVirtioSerialAddrRelease
> 
>  src/conf/domain_addr.c   | 68 
>  src/conf/domain_addr.h   |  8 -
>  src/libvirt_private.syms |  2 --
>  3 files changed, 78 deletions(-)
> 


Reviewed-by: John Ferlan 
(series, and pushed)

John

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

[libvirt] [PATCH] Post-release version bump to 4.6.0

[John Ferlan]

Signed-off-by: John Ferlan 
---
 configure.ac  | 2 +-
 docs/news.xml | 8 
 2 files changed, 9 insertions(+), 1 deletion(-)

 Pushed under trivial rule.

diff --git a/configure.ac b/configure.ac
index e25bf0a6ec..59d2d09611 100644
--- a/configure.ac
+++ b/configure.ac
@@ -16,7 +16,7 @@ dnl You should have received a copy of the GNU Lesser General 
Public
 dnl License along with this library.  If not, see
 dnl .
 
-AC_INIT([libvirt], [4.5.0], [libvir-list@redhat.com], [], 
[https://libvirt.org])
+AC_INIT([libvirt], [4.6.0], [libvir-list@redhat.com], [], 
[https://libvirt.org])
 AC_CONFIG_SRCDIR([src/libvirt.c])
 AC_CONFIG_AUX_DIR([build-aux])
 AC_CONFIG_HEADERS([config.h])
diff --git a/docs/news.xml b/docs/news.xml
index 33824ddadb..1ddfbd1df8 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -33,6 +33,14 @@
  -->
 
 
+  
+
+
+
+
+
+
+  
   
 
   
-- 
2.14.4

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

[libvirt] [PATCH] util:Fix with process number and pid file do not match

[dubo163]

dubobo (1):
  util:Fix with process number and pid file do not match

 src/util/virpidfile.c | 6 ++
 1 file changed, 6 insertions(+)

-- 
1.8.3.1

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

Re: [libvirt] [PATCH 1/2] qemu: move qemuDomainCCWAddrSetCreateFromDomain

[John Ferlan]

On 06/28/2018 04:23 PM, Anya Harter wrote:
> from src/qemu/qemu_domain_address.[ch] to src/conf/domain_addr.[ch]
> 
> Signed-off-by: Anya Harter 
> ---
>  src/conf/domain_addr.c | 24 
>  src/conf/domain_addr.h |  4 
>  src/qemu/qemu_domain_address.c | 22 --
>  src/qemu/qemu_domain_address.h |  4 
>  4 files changed, 28 insertions(+), 26 deletions(-)
> 

While I can appreciate the 2 patch approach for ease of review, patch 1
and patch 2 should be combined. Once you move a function into src/conf/*
- give it the vir* prefix.

Since you now have a function inside src/conf called from a driver, you
must add the function to src/libvirt_private.syms (otherwise your build
fails - at least mine did).

Additionally, once patches are combined virDomainCCWAddressSetCreate,
virDomainCCWAddressValidate, and virDomainCCWAddressAllocate (moved in
this patch) all become local/static to domain_addr and thus can be
removed from domain_addr.h and libvirt_private.syms

John

BTW: The ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) from the .h file that
would then be "lost" could be added after the "static int" in the .c
file, although that's not a requirement. We're finding those are really
only useful if someone pass NULL as an argument and less useful if an
argument itself is NULL (which isn't the case here).

> diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
> index 39f22b82eb..6c39608e01 100644
> --- a/src/conf/domain_addr.c
> +++ b/src/conf/domain_addr.c
> @@ -1209,6 +1209,30 @@ virDomainCCWAddressSetCreate(void)
>  }
>  
>  
> +virDomainCCWAddressSetPtr
> +qemuDomainCCWAddrSetCreateFromDomain(virDomainDefPtr def)
> +{
> +virDomainCCWAddressSetPtr addrs = NULL;
> +
> +if (!(addrs = virDomainCCWAddressSetCreate()))
> +goto error;
> +
> +if (virDomainDeviceInfoIterate(def, virDomainCCWAddressValidate,
> +   addrs) < 0)
> +goto error;
> +
> +if (virDomainDeviceInfoIterate(def, virDomainCCWAddressAllocate,
> +   addrs) < 0)
> +goto error;
> +
> +return addrs;
> +
> + error:
> +virDomainCCWAddressSetFree(addrs);
> +return NULL;
> +}
> +
> +
>  #define VIR_DOMAIN_DEFAULT_VIRTIO_SERIAL_PORTS 31
>  
>  
> diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h
> index 3236b7d6de..039efcaaf1 100644
> --- a/src/conf/domain_addr.h
> +++ b/src/conf/domain_addr.h
> @@ -205,6 +205,10 @@ int 
> virDomainCCWAddressReleaseAddr(virDomainCCWAddressSetPtr addrs,
>  ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
>  virDomainCCWAddressSetPtr virDomainCCWAddressSetCreate(void);
>  
> +virDomainCCWAddressSetPtr
> +qemuDomainCCWAddrSetCreateFromDomain(virDomainDefPtr def)
> +ATTRIBUTE_NONNULL(1);
> +
>  struct _virDomainVirtioSerialController {
>  unsigned int idx;
>  virBitmapPtr ports;
> diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
> index e9f460d77a..b1aef64d89 100644
> --- a/src/qemu/qemu_domain_address.c
> +++ b/src/qemu/qemu_domain_address.c
> @@ -376,28 +376,6 @@ qemuDomainPrimeVirtioDeviceAddresses(virDomainDefPtr def,
>  }
>  }
>  
> -virDomainCCWAddressSetPtr
> -qemuDomainCCWAddrSetCreateFromDomain(virDomainDefPtr def)
> -{
> -virDomainCCWAddressSetPtr addrs = NULL;
> -
> -if (!(addrs = virDomainCCWAddressSetCreate()))
> -goto error;
> -
> -if (virDomainDeviceInfoIterate(def, virDomainCCWAddressValidate,
> -   addrs) < 0)
> -goto error;
> -
> -if (virDomainDeviceInfoIterate(def, virDomainCCWAddressAllocate,
> -   addrs) < 0)
> -goto error;
> -
> -return addrs;
> -
> - error:
> -virDomainCCWAddressSetFree(addrs);
> -return NULL;
> -}
>  
>  /*
>   * Three steps populating CCW devnos
> diff --git a/src/qemu/qemu_domain_address.h b/src/qemu/qemu_domain_address.h
> index 83f8e81cad..89d7a5ac3e 100644
> --- a/src/qemu/qemu_domain_address.h
> +++ b/src/qemu/qemu_domain_address.h
> @@ -59,10 +59,6 @@ void qemuDomainReleaseDeviceAddress(virDomainObjPtr vm,
>  virDomainDeviceInfoPtr info,
>  const char *devstr);
>  
> -virDomainCCWAddressSetPtr
> -qemuDomainCCWAddrSetCreateFromDomain(virDomainDefPtr def)
> -ATTRIBUTE_NONNULL(1);
> -
>  int qemuDomainAssignMemoryDeviceSlot(virDomainDefPtr def,
>   virDomainMemoryDefPtr mem);
>  
> 

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

[libvirt] Release of libvirt-4.5.0

[Daniel Veillard]

  As planned the release is out, it is tagged in git and I have pushed
the signed tarball and rpms to the usual place:

   ftp://libvirt.org/libvirt/

I also made a release of the python bindings 4.5.0 also tagged in git with
signed tarball and rpms at:

   ftp://libvirt.org/libvirt/python


 This release has a distinct flavour around security, with one added feature
and improvement in that direction but also removal of some features which might
prove insecure. Beside that a potential crasher was fixed so user are invited
to update to this new version:

New features:

- qemu: Provide TPM emulator support
  Support QEMU's TPM emulator based on swtpm. Each QEMU guest gets its
  own virtual TPM.

- bhyve: Support specifying guest CPU topology
  Bhyve's guest CPU topology could be specified using the  element.

- qemu: Add support for extended TSEG size
  Support specifying extended TSEG size for SMM in QEMU.

- qemu: Add support for SEV guests
  SEV (Secure Encrypted Virtualization) is a feature available on AMD
  CPUs that encrypts the guest memory and makes it inaccessible even to
  the host OS.

Removed features:

- Remove support for qcow/default encrypted volumes
  Disallow using a qcow encrypted volume for the guest and disallow
  creation of the qcow or default encrypted volume from the storage
  driver. Support for qcow encrypted volumes has been phasing out since
  QEMU 2.3 and by QEMU 2.9 creation of a qcow encrypted volume via
  qemu-img required usage of secret objects, but that support was never
  added to libvirt.

- Make GnuTLS mandatory
  Building without GnuTLS is no longer possible.

- qemu: Remove allow_disk_format_probing configuration option
  The option represented a security risk when used with malicious disk
  images, so users were recommended against enabling it; with this
  release, it's been removed altogether.

Improvements:

- capabilities: Provide info about host IOMMU support
  Capabilities XML now provide information about host IOMMU support.

- virsh: Add --all to domblkinfo command
  Alter the domblkinfo command to add the option --all in order to
  display the size details of each domain block device from one command
  in a output table.

- qemu: Allow concurrent access to monitor and guest agent
  Historically libvirt prevented concurrent accesses to the qemu monitor
  and the guest agent. Therefore two independent calls (one querying the
  monitor and the other querying guest agent) would serialize which hurts
  performance. The code was reworked to allow two independent calls run
  at the same time.

- qemu: Allow configuring the page size for HPT pSeries guests
  For HPT pSeries guests, the size of the host pages used to back guest
  memory and the usable guest page sizes are connected; the new setting
  can be used to request that a certain page size is available in the
  guest.

- Add support to use an raw input volume for encryption
  It is now possible to provide a raw input volume as input for to
  generate a luks encrypted volume via either virsh vol-create-from or
  virStorageVolCreateXMLFrom.

- qemu: Add support for vsock hot (un)plug and cold (un)plug

- qemu: Add support for NBD over TLS
  NBD volumes can now be accessed securely.

- qemu: Implement FD passing for Unix sockets
  Instead of having QEMU open the socket and then connecting to it, which
  is inherently racy, starting with QEMU 2.12 we can open the socket
  ourselves and pass it to QEMU, avoiding race conditions.

- virsh: Introduce --nowait option for domstat command
  When this option is specified, virsh will try to fetch the guest stats
  but abort instead of stalling if they can't be retrieved right away.

Bug fixes:

- qemu: Fix a potential libvirtd crash on VM reconnect
  Initialization of the driver worker pool needs to come before libvirtd
  trying to reconnect to all machines, since one of the QEMU processes
  migh have already emitted events which need to be handled prior to us
  getting to the worker pool initialization.

- qemu: Fix domain resume after failed migration
  Recent versions of QEMU activate block devices before the guest CPU has
  been started, which makes it impossible to roll back a failed
  migration. Use the late-block-activate migration capability if
  supported to avoid the issue.

- vmx: Permit guests to have an odd number of vCPUs
  An odd number of vCPUs greater than 1 was forbidden in the past, but
  current versions of ESXi have lifted that restriction.

  Thanks everybody for your contributions to this release, be it with
code, ideas, bug reports, patch reviews, documentation, etc...

   Enjoy the release !

Daniel


-- 
Daniel Veillard  | Red Hat Developers Tools http://developer.redhat.com/
veill...@redhat.com  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | virtualization library  http://libvirt.org/

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

Re: [libvirt] [PATCH] qemu: don't use chardev FD passing with standalone args

[John Ferlan]

On 06/28/2018 07:50 AM, Daniel P. Berrangé wrote:
> When using domxml-to-native, we must generate CLI args that can be used
> in a standalone scenario. This means no FD passing can be used. To
> achieve this we must clear the QEMU_CAPS_CHARDEV_FD_PASS capability bit.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/qemu/qemu_process.c | 5 +
>  src/qemu/qemu_process.h | 2 ++
>  2 files changed, 7 insertions(+)
> 

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH 5/5] conf: Introduce new video type 'none'

[John Ferlan]

On 06/28/2018 08:15 AM, Erik Skultety wrote:
> Historically, we've always enabled an emulated video device every time we
> see that graphics should be supported with a guest. With the appearance
> of mediated devices which can support QEMU's vfio-display capability,
> users might want to use such a device as the only video device.
> Therefore introduce a new, effectively a 'disable', type for video
> device.
> 
> Signed-off-by: Erik Skultety 
> ---
>  docs/formatdomain.html.in  | 10 +++-
>  docs/schemas/domaincommon.rng  |  1 +
>  src/conf/domain_conf.c | 55 
> --
>  src/conf/domain_conf.h |  1 +
>  src/qemu/qemu_command.c| 13 +++--
>  src/qemu/qemu_domain.c |  3 ++
>  src/qemu/qemu_domain_address.c | 10 
>  tests/domaincapsschemadata/full.xml|  1 +
>  .../video-invalid-multiple-devices.xml | 33 +
>  tests/qemuxml2argvdata/video-none-device.args  | 27 +++
>  tests/qemuxml2argvdata/video-none-device.xml   | 39 +++
>  tests/qemuxml2argvtest.c   |  4 +-
>  tests/qemuxml2xmloutdata/video-none-device.xml | 42 +
>  tests/qemuxml2xmltest.c|  1 +
>  14 files changed, 219 insertions(+), 21 deletions(-)
>  create mode 100644 tests/qemuxml2argvdata/video-invalid-multiple-devices.xml
>  create mode 100644 tests/qemuxml2argvdata/video-none-device.args
>  create mode 100644 tests/qemuxml2argvdata/video-none-device.xml
>  create mode 100644 tests/qemuxml2xmloutdata/video-none-device.xml
> 

I assume this has to do with the egl-headless discussion from the other
series, so rather than "none", why not go with headless?  The problem I
see w/ NONE is it's usage/meaning typically as, well, NONE or not
defined; whereas, for graphics it would then mean, well it's defined,
but it's something else.

If this isn't a headless device, then I'll need to revisit my comments
below...

Also please split domain/docs/xml2ml from qemu/xml2argv

> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index f45eee6812..2e8196c21f 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -6639,9 +6639,15 @@ qemu-kvm -net nic,model=? /dev/null
>The model element has a mandatory type
>attribute which takes the value "vga", "cirrus", "vmvga", "xen",
>"vbox", "qxl" (since 0.8.6),
> -  "virtio" (since 1.3.0)
> -  or "gop" (since 3.2.0)
> +  "virtio" (since 1.3.0),
> +  "gop" (since 3.2.0), or
> +  "none" (since 4.6.0)
>depending on the hypervisor features available.
> +  Note that type "none" is currently only available for
> +  QEMU and the intended use case is to prevent libvirt from adding a
> +  default emulated video card in case a host device like mdev should
> +  handle the rendering, see also
> +  Graphical framebuffers.

"host device like mdev" - I assume you meant "type"... In any case, not
sure it's possible from the domain/xml2xml viewpoint to say QEMU only -
having it in domain. Perhaps consider something like:

"The headless type is designed to be used in coordination
with a host device graphics co-processor such as is provided via
Mediated Device vGPUs. When using a headless type, it must
be the only graphics device defined for the domain.

You could add a link from mdev to
https://libvirt.org/drvnodedev.html#MDEV just like the host device mdev
description did or a link to the hostdev section. It's too bad there
wasn't an anchor for that directly...



>  
>  
>You can provide the amount of video memory in kibibytes (blocks of
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 1df479cda2..55da8c079b 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -3468,6 +3468,7 @@
>  vbox
>  virtio
>  gop
> +none

headless

>
>  
>  
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 96ab6cf520..cd2a6c991c 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -589,7 +589,8 @@ VIR_ENUM_IMPL(virDomainVideo, VIR_DOMAIN_VIDEO_TYPE_LAST,
>"qxl",
>"parallels",
>"virtio",
> -  "gop")
> +  "gop",
> +  "none")

"headless"

>  
>  VIR_ENUM_IMPL(virDomainVideoVGAConf, VIR_DOMAIN_VIDEO_VGACONF_LAST,
>"io",
> @@ -5125,25 +5126,48 @@ static int
>  virDomainDefPostParseVideo(virDomainDefPtr def,
> void *opaque)
>  {
> +size_t i;
> +
>  if (def->nvideos == 0)
>  return 0;
>  
> -

Re: [libvirt] [PATCH 3/5] conf: Introduce virDomainDefPostParseVideo helper

[John Ferlan]

On 06/28/2018 08:15 AM, Erik Skultety wrote:
> Move the video post parse bits into a separate helper as the logic is
> going to be extended in the future.
> 
> Signed-off-by: Erik Skultety 
> ---
>  src/conf/domain_conf.c | 45 ++---
>  1 file changed, 30 insertions(+), 15 deletions(-)
> 

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH 4/5] qemu: validate: Enforce compile time switch type checking for videos

[John Ferlan]

On 06/28/2018 08:15 AM, Erik Skultety wrote:
> There wasn't an explicit type case to the video type enum in
> qemuDomainDeviceDefValidateVideo, _TYPE_GOP was also missing from the
> switch.
> 
> Signed-off-by: Erik Skultety 
> ---
>  src/conf/domain_conf.h | 2 +-
>  src/qemu/qemu_domain.c | 3 ++-
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH 2/5] conf: Introduce virDomainVideoDefClear helper

[John Ferlan]

On 06/28/2018 08:14 AM, Erik Skultety wrote:
> Future patches rely on the ability to reset the contents of the
> virDomainVideoDef structure rather than re-allocating it.
> 
> Signed-off-by: Erik Skultety 
> ---
>  src/conf/domain_conf.c   | 14 +-
>  src/conf/domain_conf.h   |  1 +
>  src/libvirt_private.syms |  1 +
>  3 files changed, 15 insertions(+), 1 deletion(-)
> 

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH 1/5] qemu: address: Handle all the video devices within a single loop

[John Ferlan]

On 06/28/2018 08:14 AM, Erik Skultety wrote:
> We've been handling the primary video device separately from all the
> other ones when in fact the code to do that was the same. Therefore,
> let's handle all the devices within the existing 'for' loop.
> 
> Signed-off-by: Erik Skultety 
> ---
>  src/qemu/qemu_domain_address.c | 10 ++
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 

May be worthwhile to note that since commit id 133fb140 moved validation
of the video device type, thus now it's possible to combine the PCI
address checking into one for loop.

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH v2 0/2] {lxc, util}: Fix issues with NULL argument 'type' for mount().

[Julio Faracco]

Hi Michael,

Actually, "none" is not used. It is not the proper type there, but since it
is not used, we can go ahead. If we check the documentation, we can see
the text "The filesystemtype and data arguments are ignored." in options
BIND, MOVE and REMOUNT. So, no matter which option we put, the syscall
will ignore.

--
Julio Cesar Faracco
Em qui, 28 de jun de 2018 às 05:02, Michal Prívozník
 escreveu:
>
> On 06/27/2018 05:06 PM, Julio Faracco wrote:
> > This serie fixes the argument 'type' of syscall mount(). Valgrind is
> > throwing an issue when that parameter 'type' of the syscall is NULL.
> > The best option to fix this issue is replace NULL to "none"
> > filesystem. For more details about the Valgrind bug, see the commit
> > message at 794b576c.
> >
> > Julio Faracco (2):
> >   lxc: moving 'type' argument to avoid issues with mount() syscall.
> >   util: moving 'type' argument to avoid issues with mount() syscall.
> >
> >  src/lxc/lxc_container.c | 26 +-
> >  src/util/vircgroup.c|  2 +-
> >  2 files changed, 14 insertions(+), 14 deletions(-)
> >
>
> ACK to the patches. However, I'm not quite sure about safe-for-freeze. I
> mean, patches will not break anything, but at the same time, it's not
> that important bug they are fixing, or? What is your opinion in this? Is
> is okay if I push them after the release?
>
> Michal

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

Re: [libvirt] [PATCH] news: Update for 4.5.0 release

[Andrea Bolognani]

On Mon, 2018-07-02 at 16:47 +0200, Andrea Bolognani wrote:
> Signed-off-by: Andrea Bolognani 
> ---
> I've CC'd all those who introduced the changes mentioned below,
> both to give them a chance to point out any mistake I might have
> made and to possibly annoy them into updating the release notes
> themselves next time :)
> 
>  docs/news.xml | 83 +++
>  1 file changed, 83 insertions(+)

Pushed, after addressing the comments, under the "you've had
an hour to say something and the release is coming really soon"
rule :)

-- 
Andrea Bolognani / Red Hat / Virtualization

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

[libvirt] [dbus PATCH] connect: fix g_free order in virtDBusConnectFree

[Anya Harter]

so that g_free(connect->nodeDevPath) line appears in alphabetical order
with the rest of the lines

Signed-off-by: Anya Harter 
---
 src/connect.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/connect.c b/src/connect.c
index 9ebceaa..9275121 100644
--- a/src/connect.c
+++ b/src/connect.c
@@ -1808,9 +1808,9 @@ virtDBusConnectFree(virtDBusConnect *connect)
 if (connect->connection)
 virtDBusConnectClose(connect, TRUE);
 
-g_free(connect->nodeDevPath);
 g_free(connect->domainPath);
 g_free(connect->networkPath);
+g_free(connect->nodeDevPath);
 g_free(connect->nwfilterPath);
 g_free(connect->secretPath);
 g_free(connect->storagePoolPath);
-- 
2.17.1

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

Re: [libvirt] [PATCH] news: Update for 4.5.0 release

[Pino Toscano]

On Monday, 2 July 2018 16:57:26 CEST Pino Toscano wrote:
> On Monday, 2 July 2018 16:47:00 CEST Andrea Bolognani wrote:
> > Signed-off-by: Andrea Bolognani 
> > ---
> > [...]
> > +  
> > +
> > +  vmx: Permit guests to have an odd number of vCPUs
> > +
> > +
> > +  This was forbidden in the past, but current versions of ESX can
> 
> Maybe something like:
> 
> "An odd number of guests greater than 1 was forbidden in the past,
> and current versions of ESXi support this."

Err, typos aside:

"An odd number of vCPUs greater than 1 was forbidden in the past,
and current versions of ESXi support this configuration, so the
restriction was lifted."

-- 
Pino Toscano

signature.asc
Description: This is a digitally signed message part.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] news: Update for 4.5.0 release

[Andrea Bolognani]

On Mon, 2018-07-02 at 16:53 +0200, Ján Tomko wrote:
> On Mon, Jul 02, 2018 at 04:47:00PM +0200, Andrea Bolognani wrote:
> > I've CC'd all those who introduced the changes mentioned below,
> 
> See: https://libvirt.org/hacking.html
> 
>   As a rule, patches should be sent to the mailing list only:
>   all developers are subscribed to libvir-list and read it
>   regularly, so please don't CC individual developers
>   unless they've explicitly asked you to.

Considering the fact that 4.5.0 is going to be released in just
a few hours, I felt like bending the rules a bit was appropriate.

If you want to make sure this never happens again to you, just
update the release notes as you introduce changes to libvirt ;)

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH] news: Update for 4.5.0 release

[Pino Toscano]

On Monday, 2 July 2018 16:47:00 CEST Andrea Bolognani wrote:
> Signed-off-by: Andrea Bolognani 
> ---
> [...]
> +  
> +
> +  vmx: Permit guests to have an odd number of vCPUs
> +
> +
> +  This was forbidden in the past, but current versions of ESX can

Maybe something like:

"An odd number of guests greater than 1 was forbidden in the past,
and current versions of ESXi support this."

-- 
Pino Toscano

signature.asc
Description: This is a digitally signed message part.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] news: Update for 4.5.0 release

[Peter Krempa]

On Mon, Jul 02, 2018 at 16:47:00 +0200, Andrea Bolognani wrote:
> Signed-off-by: Andrea Bolognani 
> ---
> I've CC'd all those who introduced the changes mentioned below,
> both to give them a chance to point out any mistake I might have
> made and to possibly annoy them into updating the release notes
> themselves next time :)
> 
>  docs/news.xml | 83 +++
>  1 file changed, 83 insertions(+)
> 
> diff --git a/docs/news.xml b/docs/news.xml
> index 468d34093a..65c23e51b6 100644
> --- a/docs/news.xml
> +++ b/docs/news.xml

[...]

>or virStorageVolCreateXMLFrom.
>  
>
> +  
> +
> +  qemu: Add support for vsock hot (un)plug and cold (un)plug
> +
> +  
> +  
> +
> +  qemu: Add support for NBD over TLS
> +
> +
> +  Securely accessing NBD volumes no longer requires tunnelling over
> +  SSH or another secure protocol: the native TLS support can now be
> +  used instead.

The part about "SSH or other secure protocol" does not make any sense
and also would be impossible with libvirt.

> +
> +  
> +  


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

Re: [libvirt] [PATCH] news: Update for 4.5.0 release

[Andrea Bolognani]

On Mon, 2018-07-02 at 16:51 +0200, Peter Krempa wrote:
> > +
> > +  qemu: Add support for NBD over TLS
> > +
> > +
> > +  Securely accessing NBD volumes no longer requires tunnelling over
> > +  SSH or another secure protocol: the native TLS support can now be
> > +  used instead.
> 
> The part about "SSH or other secure protocol" does not make any sense
> and also would be impossible with libvirt.

Would you mind providing an alternative wording to be used there?

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH] news: Update for 4.5.0 release

[Ján Tomko]

On Mon, Jul 02, 2018 at 04:47:00PM +0200, Andrea Bolognani wrote:

Signed-off-by: Andrea Bolognani 
---
I've CC'd all those who introduced the changes mentioned below,


See: https://libvirt.org/hacking.html

 As a rule, patches should be sent to the mailing list only:
 all developers are subscribed to libvir-list and read it
 regularly, so please don't CC individual developers
 unless they've explicitly asked you to.

Jano


both to give them a chance to point out any mistake I might have
made and to possibly annoy them into updating the release notes
themselves next time :)





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

Re: [libvirt] [PATCH] qemuDomainSaveMemory: Don't enforce dynamicOwnership

[John Ferlan]

On 06/26/2018 09:57 AM, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1589115
> 
> When doing a memory snapshot qemuOpenFile() is used. This means
> that the file where memory is saved is firstly attempted to be
> created under root:root (because that's what libvirtd is running
> under) and if this fails the second attempt is done under
> domain's uid:gid. This does not make much sense - qemu is given
> opened FD so it does not need to access the file. Moreover, if
> dynamicOwnership is set in qemu.conf and the file lives on a
> squashed NFS this is deadly combination and very likely to fail.
> 
> The fix consists of using:
> 
>   qemuOpenFileAs(fallback_uid = cfg->user,
>  fallback_gid = cfg->group,
>  dynamicOwnership = false)
> 
> In other words, dynamicOwnership is turned off for memory
> snapshot (chown() will still be attempted if the file does not
> live on NFS) and instead of using domain DAC label, configured
> user:group is set as fallback.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_driver.c | 9 ++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 

I see from reading the bz in a way you hoped to provoke upstream
discussion on this, so...

It seems the primary motivation is to go with dynamicOwnership = false
is because that "fixes" the bz for the "corner case" where someone is
saving their snapshot to a root squashed NFS share. If the same scenario
was played out for core dump or managed save image, wouldn't the same
problem exist?  Although the latter would skip the O_CREAT check in
qemuOpenFlagsAs thus not setting VIR_FILE_OPEN_FORCE_OWNER, but could
perhaps theoretically fail as IIRC the root squash case only had one way
to resolve.

Since @path_shared == 1 (only way to get to the second attempt to
virFileOpenAs), then why doesn't this code try the VIR_FILE_OPEN_FORK?
It's been a while, but IIRC that's what allowed storageBackendCreateRaw
to be successful.

IIUC the proposed solution to clear dynamicOwnership is purely to avoid
setting VIR_FILE_OPEN_FORCE_OWNER since @path_shared == 1 in the O_CREAT
path.

On a secondary note, this is the only path that doesn't pass NULL for
bypassSecuritydriver... If you follow that bypass - all that really
happens is it can alter the returned pointer, but that value is never
checked in the code - so what is it's purpose?  Taking a quick look
through history, I wonder if 23087cfdb was the last real consumer.

John

> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 129bacdd34..6af7e6e171 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -3223,6 +3223,7 @@ qemuDomainSaveMemory(virQEMUDriverPtr driver,
>   unsigned int flags,
>   qemuDomainAsyncJob asyncJob)
>  {
> +virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>  bool bypassSecurityDriver = false;
>  bool needUnlink = false;
>  int ret = -1;
> @@ -3241,9 +3242,10 @@ qemuDomainSaveMemory(virQEMUDriverPtr driver,
>  goto cleanup;
>  }
>  }
> -fd = qemuOpenFile(driver, vm, path,
> -  O_WRONLY | O_TRUNC | O_CREAT | directFlag,
> -  , );
> +
> +fd = qemuOpenFileAs(cfg->user, cfg->group, false, path,
> +O_WRONLY | O_TRUNC | O_CREAT | directFlag,
> +, );
>  if (fd < 0)
>  goto cleanup;
>  
> @@ -3283,6 +3285,7 @@ qemuDomainSaveMemory(virQEMUDriverPtr driver,
>   cleanup:
>  VIR_FORCE_CLOSE(fd);
>  virFileWrapperFdFree(wrapperFd);
> +virObjectUnref(cfg);
>  
>  if (ret < 0 && needUnlink)
>  unlink(path);
> 

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

[libvirt] [PATCH] news: Update for 4.5.0 release

[Andrea Bolognani]

Signed-off-by: Andrea Bolognani 
---
I've CC'd all those who introduced the changes mentioned below,
both to give them a chance to point out any mistake I might have
made and to possibly annoy them into updating the release notes
themselves next time :)

 docs/news.xml | 83 +++
 1 file changed, 83 insertions(+)

diff --git a/docs/news.xml b/docs/news.xml
index 468d34093a..65c23e51b6 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -61,6 +61,16 @@
   Support specifying extended TSEG size for SMM in QEMU.
 
   
+  
+
+  qemu: Add support for SEV guests
+
+
+  SEV (Secure Encrypted Virtualization) is a feature available on AMD
+  CPUs that encrypts the guest memory and makes it inaccessible even
+  to the host OS.
+
+  
 
 
   
@@ -76,6 +86,24 @@
   secret objects, but that support was never added to libvirt.
 
   
+  
+
+  Make GnuTLS mandatory
+
+
+  Building without GnuTLS is no longer possible.
+
+  
+  
+
+  qemu: Remove allow_disk_format_probing configuration option
+
+
+  The option represented a security risk when used with malicious
+  disk images, so users were recommended against enabling it; with
+  this release, it's been removed altogether.
+
+  
 
 
   
@@ -130,6 +158,41 @@
   or virStorageVolCreateXMLFrom.
 
   
+  
+
+  qemu: Add support for vsock hot (un)plug and cold (un)plug
+
+  
+  
+
+  qemu: Add support for NBD over TLS
+
+
+  Securely accessing NBD volumes no longer requires tunnelling over
+  SSH or another secure protocol: the native TLS support can now be
+  used instead.
+
+  
+  
+
+  qemu: Implement FD passing for Unix sockets
+
+
+  Instead of having QEMU open the socket and then connecting to it,
+  which is inherently racy, starting with QEMU 2.12 we can open the
+  socket ourselves and pass it to QEMU, avoiding race conditions.
+
+  
+  
+
+  virsh: Introduce --nowait option for domstat command
+
+
+  When this option is specified, virsh will try to fetch the guest
+  stats but abort instead of stalling if they can't be retrieved right
+  away.
+
+  
 
 
   
@@ -143,6 +206,26 @@
   us getting to the worker pool initialization.
 
   
+  
+
+  qemu: Fix domain resume after failed migration
+
+
+  Recent versions of QEMU activate block devices before the guest CPU
+  has been started, which makes it impossible to roll back a failed
+  migration. Use the late-block-activate migration
+  capability if supported to avoid the issue.
+
+  
+  
+
+  vmx: Permit guests to have an odd number of vCPUs
+
+
+  This was forbidden in the past, but current versions of ESX can
+  deal with a guest configured as such.
+
+  
 
   
   
-- 
2.17.1

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

Re: [libvirt] [PATCH 3/5] virStorageBackendIQNFound: Rework iscsiadm output parsing

[Michal Prívozník]

On 07/02/2018 09:39 AM, Peter Krempa wrote:
> On Fri, Jun 29, 2018 at 17:01:49 +0200, Michal Privoznik wrote:
>> Firstly, we can utilize virCommandSetOutputBuffer() API which
>> will collect the command output for us. Secondly, sscanf()-ing
>> through each line is easier to understand (and more robust) than
>> jumping over a string with strchr().
>>
>> Signed-off-by: Michal Privoznik 
>> ---
>>  src/util/viriscsi.c | 85 
>> +
>>  1 file changed, 34 insertions(+), 51 deletions(-)
>>
>> diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c
>> index 2e55b3c10b..44788056fd 100644
>> --- a/src/util/viriscsi.c
>> +++ b/src/util/viriscsi.c
> 
> [...]
> 
>> @@ -117,71 +116,56 @@ static int
>>  virStorageBackendIQNFound(const char *initiatoriqn,
>>char **ifacename)
>>  {
>> -int ret = IQN_ERROR, fd = -1;
>> -char ebuf[64];
>> -FILE *fp = NULL;
>> -char *line = NULL, *newline = NULL, *iqn = NULL, *token = NULL;
>> +int ret = IQN_ERROR;
>> +char *outbuf = NULL;
>> +char *line = NULL;
>> +char *iface = NULL;
>> +char *iqn = NULL;
>>  virCommandPtr cmd = virCommandNewArgList(ISCSIADM,
>>   "--mode", "iface", NULL);
>>  
>>  *ifacename = NULL;
>>  
>> -if (VIR_ALLOC_N(line, LINE_SIZE) != 0) {
>> -virReportError(VIR_ERR_INTERNAL_ERROR,
>> -   _("Could not allocate memory for output of '%s'"),
>> -   ISCSIADM);
>> +virCommandSetOutputBuffer(cmd, );
>> +if (virCommandRun(cmd, NULL) < 0)
>>  goto cleanup;
>> -}
>>  
>> -memset(line, 0, LINE_SIZE);
>> +/* Example of data we are dealing with:
>> + * default tcp
>> + * iser iser
>> + * libvirt-iface-253db048 
>> tcpiqn.2017-03.com.user:client
>> + */
>>  
>> -virCommandSetOutputFD(cmd, );
>> -if (virCommandRunAsync(cmd, NULL) < 0)
>> -goto cleanup;
>> +line = outbuf;
>> +while (line) {
> 
> This is severely misleading and makes this loop technically infinite. Or
> just checks that output buffer was not null. Both operations should be
> made explicit. Or you meant *line or line[0]

Okay, how about:

while (line && *line) {

Michal

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

Re: [libvirt] [PATCH v3] qemu: format serial and geometry on frontend disk device

[Daniel P . Berrangé]

On Mon, Jul 02, 2018 at 02:38:52PM +0200, Peter Krempa wrote:
> On Mon, Jul 02, 2018 at 13:35:49 +0100, Daniel Berrange wrote:
> > On Mon, Jul 02, 2018 at 02:31:31PM +0200, Peter Krempa wrote:
> > > On Mon, Jul 02, 2018 at 10:17:07 +0100, Daniel Berrange wrote:
> > > > Currently we format the serial, geometry and error policy on the -drive
> > > > backend argument.
> > > > 
> > > > QEMU added the ability to set serial and geometry on the frontend in
> > > > the 1.2 release deprecating use of -drive, with support being deleted
> > > > from -drive in 3.0.
> > > > 
> > > > We keep formatting error policy on -drive for now, because we don't
> > > > ahve support for that with -device for usb-storage just yet.
> > > > 
> > > > Note that some disk buses (sd) still don't support -device. Although
> > > > QEMU allowed these properties to be set on -drive for if=sd, they
> > > > have been ignored so we now report an error in this case.
> > > > 
> > > > Signed-off-by: Daniel P. Berrangé 
> > > > ---
> > > > 
> > > > Changed in v3:
> > > > 
> > > >  - Report error for setting CHS on USB
> > > >  - Report error for setting CHS translation mode for bus != IDE
> > > >  - Use 'bios-chs-trans' property not 'trans'
> > > > 
> > > >  src/qemu/qemu_command.c   | 46 ---
> > > >  .../disk-drive-network-tlsx509.args   | 12 ++---
> > > >  .../disk-drive-network-vxhs.args  |  4 +-
> > > >  tests/qemuxml2argvdata/disk-drive-shared.args |  5 +-
> > > >  tests/qemuxml2argvdata/disk-geometry.args |  6 +--
> > > >  tests/qemuxml2argvdata/disk-ide-wwn.args  |  5 +-
> > > >  .../qemuxml2argvdata/disk-scsi-disk-wwn.args  |  4 +-
> > > >  tests/qemuxml2argvdata/disk-serial.args   | 10 ++--
> > > >  tests/qemuxml2argvdata/disk-serial.xml|  1 -
> > > >  tests/qemuxml2xmloutdata/disk-serial.xml  |  1 -
> > > >  10 files changed, 62 insertions(+), 32 deletions(-)
> > > > 
> > > > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > > > index 4fc3176ad3..46726b055e 100644
> > > > --- a/src/qemu/qemu_command.c
> > > > +++ b/src/qemu/qemu_command.c
> > > > @@ -1599,7 +1599,7 @@ 
> > > > qemuBuildDiskFrontendAttributeErrorPolicy(virDomainDiskDefPtr disk,
> > > >  }
> > > >  
> > > >  
> > > > -static void
> > > > +static int
> > > >  qemuBuildDiskFrontendAttributes(virDomainDiskDefPtr disk,
> > > >  virBufferPtr buf)
> > > >  {
> > > > @@ -1607,14 +1607,27 @@ 
> > > > qemuBuildDiskFrontendAttributes(virDomainDiskDefPtr disk,
> > > >  if (disk->geometry.cylinders > 0 &&
> > > >  disk->geometry.heads > 0 &&
> > > >  disk->geometry.sectors > 0) {
> > > > +if (disk->bus == VIR_DOMAIN_DISK_BUS_USB) {
> > > > +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > > > +   _("CHS geometry can not be set for 'usb' 
> > > > bus"));
> > > > +return -1;
> > > > +}
> > > 
> > > I'm not a fan of validation in the command line generator. I'd prefer if
> > > you could move this to qemuDomainDeviceDefValidateDisk so that it gets
> > > rejected at define time.
> > 
> > Won't that cause us to fail to load the VM definition when upgrading 
> > existing
> > libvirt.
> 
> No. The validation callbacks are not called for existing configs on
> disk. They are re-called on vm startup though so that any existing
> config gets surely validated.

Ok, that's great, I'll make such a change.


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 v3] qemu: format serial and geometry on frontend disk device

[Peter Krempa]

On Mon, Jul 02, 2018 at 13:35:49 +0100, Daniel Berrange wrote:
> On Mon, Jul 02, 2018 at 02:31:31PM +0200, Peter Krempa wrote:
> > On Mon, Jul 02, 2018 at 10:17:07 +0100, Daniel Berrange wrote:
> > > Currently we format the serial, geometry and error policy on the -drive
> > > backend argument.
> > > 
> > > QEMU added the ability to set serial and geometry on the frontend in
> > > the 1.2 release deprecating use of -drive, with support being deleted
> > > from -drive in 3.0.
> > > 
> > > We keep formatting error policy on -drive for now, because we don't
> > > ahve support for that with -device for usb-storage just yet.
> > > 
> > > Note that some disk buses (sd) still don't support -device. Although
> > > QEMU allowed these properties to be set on -drive for if=sd, they
> > > have been ignored so we now report an error in this case.
> > > 
> > > Signed-off-by: Daniel P. Berrangé 
> > > ---
> > > 
> > > Changed in v3:
> > > 
> > >  - Report error for setting CHS on USB
> > >  - Report error for setting CHS translation mode for bus != IDE
> > >  - Use 'bios-chs-trans' property not 'trans'
> > > 
> > >  src/qemu/qemu_command.c   | 46 ---
> > >  .../disk-drive-network-tlsx509.args   | 12 ++---
> > >  .../disk-drive-network-vxhs.args  |  4 +-
> > >  tests/qemuxml2argvdata/disk-drive-shared.args |  5 +-
> > >  tests/qemuxml2argvdata/disk-geometry.args |  6 +--
> > >  tests/qemuxml2argvdata/disk-ide-wwn.args  |  5 +-
> > >  .../qemuxml2argvdata/disk-scsi-disk-wwn.args  |  4 +-
> > >  tests/qemuxml2argvdata/disk-serial.args   | 10 ++--
> > >  tests/qemuxml2argvdata/disk-serial.xml|  1 -
> > >  tests/qemuxml2xmloutdata/disk-serial.xml  |  1 -
> > >  10 files changed, 62 insertions(+), 32 deletions(-)
> > > 
> > > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > > index 4fc3176ad3..46726b055e 100644
> > > --- a/src/qemu/qemu_command.c
> > > +++ b/src/qemu/qemu_command.c
> > > @@ -1599,7 +1599,7 @@ 
> > > qemuBuildDiskFrontendAttributeErrorPolicy(virDomainDiskDefPtr disk,
> > >  }
> > >  
> > >  
> > > -static void
> > > +static int
> > >  qemuBuildDiskFrontendAttributes(virDomainDiskDefPtr disk,
> > >  virBufferPtr buf)
> > >  {
> > > @@ -1607,14 +1607,27 @@ 
> > > qemuBuildDiskFrontendAttributes(virDomainDiskDefPtr disk,
> > >  if (disk->geometry.cylinders > 0 &&
> > >  disk->geometry.heads > 0 &&
> > >  disk->geometry.sectors > 0) {
> > > +if (disk->bus == VIR_DOMAIN_DISK_BUS_USB) {
> > > +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > > +   _("CHS geometry can not be set for 'usb' 
> > > bus"));
> > > +return -1;
> > > +}
> > 
> > I'm not a fan of validation in the command line generator. I'd prefer if
> > you could move this to qemuDomainDeviceDefValidateDisk so that it gets
> > rejected at define time.
> 
> Won't that cause us to fail to load the VM definition when upgrading existing
> libvirt.

No. The validation callbacks are not called for existing configs on
disk. They are re-called on vm startup though so that any existing
config gets surely validated.


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

Re: [libvirt] [PATCH v3] qemu: format serial and geometry on frontend disk device

[Daniel P . Berrangé]

On Mon, Jul 02, 2018 at 02:31:31PM +0200, Peter Krempa wrote:
> On Mon, Jul 02, 2018 at 10:17:07 +0100, Daniel Berrange wrote:
> > Currently we format the serial, geometry and error policy on the -drive
> > backend argument.
> > 
> > QEMU added the ability to set serial and geometry on the frontend in
> > the 1.2 release deprecating use of -drive, with support being deleted
> > from -drive in 3.0.
> > 
> > We keep formatting error policy on -drive for now, because we don't
> > ahve support for that with -device for usb-storage just yet.
> > 
> > Note that some disk buses (sd) still don't support -device. Although
> > QEMU allowed these properties to be set on -drive for if=sd, they
> > have been ignored so we now report an error in this case.
> > 
> > Signed-off-by: Daniel P. Berrangé 
> > ---
> > 
> > Changed in v3:
> > 
> >  - Report error for setting CHS on USB
> >  - Report error for setting CHS translation mode for bus != IDE
> >  - Use 'bios-chs-trans' property not 'trans'
> > 
> >  src/qemu/qemu_command.c   | 46 ---
> >  .../disk-drive-network-tlsx509.args   | 12 ++---
> >  .../disk-drive-network-vxhs.args  |  4 +-
> >  tests/qemuxml2argvdata/disk-drive-shared.args |  5 +-
> >  tests/qemuxml2argvdata/disk-geometry.args |  6 +--
> >  tests/qemuxml2argvdata/disk-ide-wwn.args  |  5 +-
> >  .../qemuxml2argvdata/disk-scsi-disk-wwn.args  |  4 +-
> >  tests/qemuxml2argvdata/disk-serial.args   | 10 ++--
> >  tests/qemuxml2argvdata/disk-serial.xml|  1 -
> >  tests/qemuxml2xmloutdata/disk-serial.xml  |  1 -
> >  10 files changed, 62 insertions(+), 32 deletions(-)
> > 
> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > index 4fc3176ad3..46726b055e 100644
> > --- a/src/qemu/qemu_command.c
> > +++ b/src/qemu/qemu_command.c
> > @@ -1599,7 +1599,7 @@ 
> > qemuBuildDiskFrontendAttributeErrorPolicy(virDomainDiskDefPtr disk,
> >  }
> >  
> >  
> > -static void
> > +static int
> >  qemuBuildDiskFrontendAttributes(virDomainDiskDefPtr disk,
> >  virBufferPtr buf)
> >  {
> > @@ -1607,14 +1607,27 @@ qemuBuildDiskFrontendAttributes(virDomainDiskDefPtr 
> > disk,
> >  if (disk->geometry.cylinders > 0 &&
> >  disk->geometry.heads > 0 &&
> >  disk->geometry.sectors > 0) {
> > +if (disk->bus == VIR_DOMAIN_DISK_BUS_USB) {
> > +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > +   _("CHS geometry can not be set for 'usb' bus"));
> > +return -1;
> > +}
> 
> I'm not a fan of validation in the command line generator. I'd prefer if
> you could move this to qemuDomainDeviceDefValidateDisk so that it gets
> rejected at define time.

Won't that cause us to fail to load the VM definition when upgrading existing
libvirt.

> 
> > +
> >  virBufferAsprintf(buf, ",cyls=%u,heads=%u,secs=%u",
> >disk->geometry.cylinders,
> >disk->geometry.heads,
> >disk->geometry.sectors);
> >  
> > -if (disk->geometry.trans != VIR_DOMAIN_DISK_TRANS_DEFAULT)
> > -virBufferAsprintf(buf, ",trans=%s",
> > +if (disk->geometry.trans != VIR_DOMAIN_DISK_TRANS_DEFAULT) {
> > +if (disk->bus != VIR_DOMAIN_DISK_BUS_IDE) {
> > +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > +   _("CHS translation mode can only be set for 
> > 'ide' bus not '%s'"),
> > +   virDomainDiskBusTypeToString(disk->bus));
> > +return -1;
> 
> This would be a good candidate too.
> 
> 
> > +}
> > +virBufferAsprintf(buf, ",bios-chs-trans=%s",
> >
> > virDomainDiskGeometryTransTypeToString(disk->geometry.trans));
> > +}
> >  }
> >  
> >  if (disk->serial) {
> > @@ -1622,7 +1635,7 @@ qemuBuildDiskFrontendAttributes(virDomainDiskDefPtr 
> > disk,
> >  virBufferEscape(buf, '\\', " ", "%s", disk->serial);
> >  }
> >  
> > -qemuBuildDiskFrontendAttributeErrorPolicy(disk, buf);
> > +return 0;
> >  }
> >  
> >  
> > @@ -1662,11 +1675,27 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
> >  virBufferAsprintf(, "if=%s",
> >virDomainDiskQEMUBusTypeToString(disk->bus));
> >  virBufferAsprintf(, ",index=%d", idx);
> > +
> > +if (disk->serial) {
> > +virReportError(VIR_ERR_INTERNAL_ERROR,
> > +   _("Serial property not supported for drive bus 
> > '%s'"),
> > +   virDomainDiskBusTypeToString(disk->bus));
> > +goto error;
> > +}
> > +if (disk->geometry.cylinders > 0 &&
> > +disk->geometry.heads > 0 &&
> > +disk->geometry.sectors > 0) {
> > +virReportError(VIR_ERR_INTERNAL_ERROR,
> > +   _("Geometry 

Re: [libvirt] [PATCH v3] qemu: format serial and geometry on frontend disk device

[Peter Krempa]

On Mon, Jul 02, 2018 at 10:17:07 +0100, Daniel Berrange wrote:
> Currently we format the serial, geometry and error policy on the -drive
> backend argument.
> 
> QEMU added the ability to set serial and geometry on the frontend in
> the 1.2 release deprecating use of -drive, with support being deleted
> from -drive in 3.0.
> 
> We keep formatting error policy on -drive for now, because we don't
> ahve support for that with -device for usb-storage just yet.
> 
> Note that some disk buses (sd) still don't support -device. Although
> QEMU allowed these properties to be set on -drive for if=sd, they
> have been ignored so we now report an error in this case.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
> 
> Changed in v3:
> 
>  - Report error for setting CHS on USB
>  - Report error for setting CHS translation mode for bus != IDE
>  - Use 'bios-chs-trans' property not 'trans'
> 
>  src/qemu/qemu_command.c   | 46 ---
>  .../disk-drive-network-tlsx509.args   | 12 ++---
>  .../disk-drive-network-vxhs.args  |  4 +-
>  tests/qemuxml2argvdata/disk-drive-shared.args |  5 +-
>  tests/qemuxml2argvdata/disk-geometry.args |  6 +--
>  tests/qemuxml2argvdata/disk-ide-wwn.args  |  5 +-
>  .../qemuxml2argvdata/disk-scsi-disk-wwn.args  |  4 +-
>  tests/qemuxml2argvdata/disk-serial.args   | 10 ++--
>  tests/qemuxml2argvdata/disk-serial.xml|  1 -
>  tests/qemuxml2xmloutdata/disk-serial.xml  |  1 -
>  10 files changed, 62 insertions(+), 32 deletions(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 4fc3176ad3..46726b055e 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -1599,7 +1599,7 @@ 
> qemuBuildDiskFrontendAttributeErrorPolicy(virDomainDiskDefPtr disk,
>  }
>  
>  
> -static void
> +static int
>  qemuBuildDiskFrontendAttributes(virDomainDiskDefPtr disk,
>  virBufferPtr buf)
>  {
> @@ -1607,14 +1607,27 @@ qemuBuildDiskFrontendAttributes(virDomainDiskDefPtr 
> disk,
>  if (disk->geometry.cylinders > 0 &&
>  disk->geometry.heads > 0 &&
>  disk->geometry.sectors > 0) {
> +if (disk->bus == VIR_DOMAIN_DISK_BUS_USB) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +   _("CHS geometry can not be set for 'usb' bus"));
> +return -1;
> +}

I'm not a fan of validation in the command line generator. I'd prefer if
you could move this to qemuDomainDeviceDefValidateDisk so that it gets
rejected at define time.

> +
>  virBufferAsprintf(buf, ",cyls=%u,heads=%u,secs=%u",
>disk->geometry.cylinders,
>disk->geometry.heads,
>disk->geometry.sectors);
>  
> -if (disk->geometry.trans != VIR_DOMAIN_DISK_TRANS_DEFAULT)
> -virBufferAsprintf(buf, ",trans=%s",
> +if (disk->geometry.trans != VIR_DOMAIN_DISK_TRANS_DEFAULT) {
> +if (disk->bus != VIR_DOMAIN_DISK_BUS_IDE) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   _("CHS translation mode can only be set for 
> 'ide' bus not '%s'"),
> +   virDomainDiskBusTypeToString(disk->bus));
> +return -1;

This would be a good candidate too.


> +}
> +virBufferAsprintf(buf, ",bios-chs-trans=%s",
>
> virDomainDiskGeometryTransTypeToString(disk->geometry.trans));
> +}
>  }
>  
>  if (disk->serial) {
> @@ -1622,7 +1635,7 @@ qemuBuildDiskFrontendAttributes(virDomainDiskDefPtr 
> disk,
>  virBufferEscape(buf, '\\', " ", "%s", disk->serial);
>  }
>  
> -qemuBuildDiskFrontendAttributeErrorPolicy(disk, buf);
> +return 0;
>  }
>  
>  
> @@ -1662,11 +1675,27 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
>  virBufferAsprintf(, "if=%s",
>virDomainDiskQEMUBusTypeToString(disk->bus));
>  virBufferAsprintf(, ",index=%d", idx);
> +
> +if (disk->serial) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("Serial property not supported for drive bus 
> '%s'"),
> +   virDomainDiskBusTypeToString(disk->bus));
> +goto error;
> +}
> +if (disk->geometry.cylinders > 0 &&
> +disk->geometry.heads > 0 &&
> +disk->geometry.sectors > 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("Geometry not supported for drive bus '%s'"),
> +   virDomainDiskBusTypeToString(disk->bus));
> +goto error;
> +}

I don't care that much about the -drive part though since that will
become almost unused.

>  }
>  
> -/* Format attributes for the drive itself (not the storage backing it) 
> which
> - * we've formatted historically with 

Re: [libvirt] [PATCH] util: return generic error in virCopyLastError if error is not set

[Nikolay Shirokovskiy]

On 18.05.2018 13:14, Erik Skultety wrote:
> On Thu, May 17, 2018 at 03:24:49PM +0300, Nikolay Shirokovskiy wrote:
>>
>>
>> On 17.05.2018 14:49, Nikolay Shirokovskiy wrote:
>>>
>>>
>>> On 17.05.2018 14:01, Erik Skultety wrote:
 On Thu, May 17, 2018 at 01:42:36PM +0300, Nikolay Shirokovskiy wrote:
>
>
> On 17.05.2018 13:11, Nikolay Shirokovskiy wrote:
>> virCopyLastError is intended to be used after last error is set.
>> However due to virLastErrorObject failures (very unlikely through
>> as thread local error is allocated on first use) we can have zero
>> fields in a copy as a result. In particular code field can be set
>> to VIR_ERR_OK.
>>
>> In some places (qemu monitor, qemu agent and qemu migaration code
>> for example) we use copy result as a flag and this leads to bugs.
>>
>> Let's set copy result to generic error ("cause unknown") in this case.
>> Approach is based on John Ferlan comments in [1] and [2] to initial
>> attempt which fixes issue in much less generic way.
>>
>> [1] https://www.redhat.com/archives/libvir-list/2018-April/msg02700.html
>> [2] https://www.redhat.com/archives/libvir-list/2018-May/msg00124.html
>> ---
>>  src/util/virerror.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/util/virerror.c b/src/util/virerror.c
>> index c000b00..9f158af 100644
>> --- a/src/util/virerror.c
>> +++ b/src/util/virerror.c
>> @@ -343,7 +343,7 @@ virCopyLastError(virErrorPtr to)
>>  if (err)
>>  virCopyError(err, to);
>>  else
>> -virResetError(to);
>> +virErrorGenericFailure(err);
>
> virErrorGenericFailure(to) of course

 Yeah, I just prepared a mail to respond to the original patch, nevermind 
 :)...
 Although I still don't see how this solves anything. First of all, the 
 whole
 else branch is useless, since we "memset'd" the caller-passed object right
 before we could potentially hit the 'else' branch. As you've noted in the
 commit message, virLastErrorObject() can only fail with NULL on OOM, yet 
 one of
 the things virErrorGenericFailure will try to do is strdup(), which I 
 highly
 doubt would succeed after a previous OOM, IOW if libvirt encounters OOM 
 it's
 pretty much stuck in a loop of failed attempts to report an OOM error
 encountering some more. If you really want to return something, I'd say we
 should return VIR_ERR_NO_MEMORY instead, probably checking the validity of 
 the
>>>
>>> This can be misleading. The original error is lost due OOM but it can be
>>> not OOM itself and we tend to report root cause. I'd prefer new 
>>> VIR_ERR_UNKNOWN code
>>> if sematics is important. Then we need to fix virErrorGenericFailure too
>>> as VIR_ERR_INTERNAL_ERROR code is valid only if we forget to set error but
>>> we can also have OOM issues too in this case, so VIR_ERR_UNKNOWN is better 
>>> suited
>>> for virErrorGenericFailure. Or we can live with just VIR_ERR_INTERNAL_ERROR.
>>>
>>> As to duping message - it won't hurt anyway. So I would keep 
>>> virErrorGenericFailure
>>> usage.
>>>
 caller-passed object, since we just blindly trust it's been allocated 
 properly
 and access it right away.

>>>
>>> As to checking @to validity, I'd prefer a distinct patch.
>>>
>>> Nikolay
>>>
>>>
>>
>> Eventually I think we just need to initialize thread local error storage
>> in the very beginning of thread life. Then we don't have this corner
>> case. And similar others too. Check for example 
>> virErrorPreserveLast/virErrorRestore.
>> They don't account that last error can be NULL because of OOM so last
>> error can be overwritten.
> 
> I think it would make sense for us to create the error object right at the
> thread creation time rather than ad-hoc when we actually need it, although, 
> how
> likely is it that we're going to hit such a corner case, but sure, feel free 
> to
> post patches for that.
> 
> Erik
> 

Hi, Erik. While this approach looks interesting it takes too much effort
to use with thread pools. We need to teach thread pool to deal with threads
than fail to initialize. It is much simplier to adopt virCopyLastError for
such a corner case. Of course we still can have issues with 
virErrorPreserveLast/virErrorRestore
but they are not critical - client just get some unrelated error but 
daemon will continue to work properly. So I sent v2 version of this patch with
changes as you suggested.

Nikolay

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

Re: [libvirt] [PATCH v3 2/3] qemu: Implement the HTM pSeries feature

[John Ferlan]

On 07/02/2018 04:19 AM, Andrea Bolognani wrote:
> On Sat, 2018-06-30 at 07:23 -0400, John Ferlan wrote:
>> On 06/26/2018 06:21 AM, Andrea Bolognani wrote:
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1525599
>>> Signed-off-by: Andrea Bolognani 
>>> ---
>>>  docs/formatdomain.html.in |  8 
>>>  docs/schemas/domaincommon.rng |  5 +
>>>  src/conf/domain_conf.c| 19 ++
>>>  src/conf/domain_conf.h|  1 +
>>>  src/qemu/qemu_command.c   | 20 +++
>>>  src/qemu/qemu_domain.c| 13 
>>>  tests/qemuxml2argvdata/pseries-features.args  |  2 +-
>>>  tests/qemuxml2argvdata/pseries-features.xml   |  1 +
>>>  tests/qemuxml2argvtest.c  |  1 +
>>>  tests/qemuxml2xmloutdata/pseries-features.xml |  1 +
>>>  tests/qemuxml2xmltest.c   |  1 +
>>>  11 files changed, 71 insertions(+), 1 deletion(-)
>>
>> Even though it's QEMU/KVM only - the conf/docs/xml2xml should be
>> separated from the qemu/xml2argv changes.
> 
> Can do.
> 
>>> @@ -2162,6 +2163,13 @@
>>>Enable QEMU vmcoreinfo device to let the guest kernel save debug
>>>details. Since 4.4.0 (QEMU only)
>>>
>>> +  htm
>>> +  Configure HTM (Hardware Transational Memory) availability for
>>> +  pSeries guests. Possible values for the state 
>>> attribute
>>> +  are on and off. If the attribute is not
>>> +  defined, the hypervisor default will be used.
>>> +  Since 4.5.0 (QEMU/KVM only)
>>
>> This'll miss 4.5, so change to 4.6 before pushing
> 
> Sure.
> 
>>> +case VIR_DOMAIN_FEATURE_HTM:
>>> +if (!(tmp = virXMLPropString(nodes[i], "state"))) {
>>> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>>> +   _("missing state attribute '%s' of feature 
>>> '%s'"),
>>> +   tmp, virDomainFeatureTypeToString(val));
>>> +goto error;
>>> +}
>>> +if ((def->features[val] = 
>>> virTristateSwitchTypeFromString(tmp)) < 0) {
>>
>> [1] checked here, so that...
>>
>>> +if (def->features[VIR_DOMAIN_FEATURE_HTM] != 
>>> VIR_TRISTATE_SWITCH_ABSENT) {
>>> +const char *str;
>>> +
>>> +if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_PSERIES_CAP_HTM)) {
>>> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>>> +   _("HTM configuration is not supported by this "
>>> + "QEMU binary"));
>>> +goto cleanup;
>>> +}
>>> +
>>> +str = 
>>> virTristateSwitchTypeToString(def->features[VIR_DOMAIN_FEATURE_HTM]);
>>> +if (!str) {
>>
>> [1] ... this is unnecessary due to virDomainDefParseXML check.
> 
> I strongly dislike the practice of skipping checks in a function
> with the rationale that they're already performed somewhere else in
> the code base: libvirt is in a state of constant flux, and as stuff
> gets moved around you might very well find yourself no longer as
> shielded from invalid values as you thought you were. Local sanity
> checks should IMHO always be performed locally.
> 
> tl;dr I'd really rather keep this in.
> 

That's fine - I don't mind the check, just pointing out it should be
unnecessary. You can leave it in and the R-by still stands.

John

htm vs. hpt, yep the tla's and seeing pseries with memory related
functionality for both certainly led me down that path...

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

Re: [libvirt] [PATCH v1 00/11] Enable vfio-pci 'property' for mediated device

[Gerd Hoffmann]

  Hi,

> > So why would we want to treat egl-headless differently as an attribute under
> > the SPICE graphics, as opposed to just having  
> > as a thing in its own right.
> 
> Egl-headless doesn't make sense on its own, neither does it make sense with
> anything else but SPICE and VNC.

Well, you can run a vm with egl-headless and without vnc/spice just
fine.  Might even make sense to do that for automated QA runs as you can
get the display content using the screendump monitor command.

So the "" suggestion looks good to me.

cheers,
  Gerd

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

[libvirt] [PATCH v2] util: set OOM in virCopyLastError if error is not set

[Nikolay Shirokovskiy]

virCopyLastError is intended to be used after last error is set.
However due to virLastErrorObject failures (very unlikely through
as thread local error is allocated on first use) we can have zero
fields in a copy as a result. In particular code field can be set
to VIR_ERR_OK.

In some places (qemu monitor, qemu agent and qemu migaration code
for example) we use copy result as a flag and this leads to bugs.

Let's set OOM-like error in copy in case of virLastErrorObject failures.

Signed-off-by: Nikolay Shirokovskiy 
---

Changes from v1:
- check @to
- set OMM error instead of using virErrorGenericFailure

 src/util/virerror.c | 16 +++-
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/src/util/virerror.c b/src/util/virerror.c
index f198f27..737ea92 100644
--- a/src/util/virerror.c
+++ b/src/util/virerror.c
@@ -366,19 +366,25 @@ virSetError(virErrorPtr newerr)
  *
  * One will need to free the result with virResetError()
  *
- * Returns 0 if no error was found and the error code otherwise and -1 in case
- * of parameter error.
+ * Returns error code or -1 in case of parameter error.
  */
 int
 virCopyLastError(virErrorPtr to)
 {
 virErrorPtr err = virLastErrorObject();
+
+if (!to)
+return -1;
+
 /* We can't guarantee caller has initialized it to zero */
 memset(to, 0, sizeof(*to));
-if (err)
+if (err) {
 virCopyError(err, to);
-else
-virResetError(to);
+} else {
+to->code = VIR_ERR_NO_MEMORY;
+to->domain = VIR_FROM_NONE;
+to->level = VIR_ERR_ERROR;
+}
 return to->code;
 }
 
-- 
1.8.3.1

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

[libvirt] [PATCH v3 1/1] qemu: Add entry for balloon stat stat-disk-caches

[Tomáš Golembiovský]

QEMU commit bf1e7140e adds reporting of new balloon statistic to QEMU
2.12. Value represents the amount of memory that can be quickly
reclaimed without additional I/O. Let's add that too.

Signed-off-by: Tomáš Golembiovský 
---
 include/libvirt/libvirt-domain.h | 9 -
 src/libvirt-domain.c | 3 +++
 src/qemu/qemu_driver.c   | 1 +
 src/qemu/qemu_monitor_json.c | 2 ++
 tools/virsh-domain-monitor.c | 2 ++
 tools/virsh.pod  | 5 +
 6 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index 796f2e1408..fdd2d6b8ea 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -628,11 +628,18 @@ typedef enum {
 /* Timestamp of the last update of statistics, in seconds. */
 VIR_DOMAIN_MEMORY_STAT_LAST_UPDATE = 9,
 
+/*
+ * The amount of memory, that can be quickly reclaimed without
+ * additional I/O (in kB). Typically these pages are used for caching files
+ * from disk.
+ */
+VIR_DOMAIN_MEMORY_STAT_DISK_CACHES = 10,
+
 /*
  * The number of statistics supported by this version of the interface.
  * To add new statistics, add them to the enum and increase this value.
  */
-VIR_DOMAIN_MEMORY_STAT_NR  = 10,
+VIR_DOMAIN_MEMORY_STAT_NR  = 11,
 
 # ifdef VIR_ENUM_SENTINELS
 VIR_DOMAIN_MEMORY_STAT_LAST = VIR_DOMAIN_MEMORY_STAT_NR
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index c71f2e6877..ef39361c95 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -5732,6 +5732,9 @@ virDomainGetInterfaceParameters(virDomainPtr domain,
  * Current balloon value (in kb).
  * VIR_DOMAIN_MEMORY_STAT_LAST_UPDATE
  * Timestamp of the last statistic
+ * VIR_DOMAIN_MEMORY_STAT_DISK_CACHES
+ * Memory that can be reclaimed without additional I/O, typically disk
+ * caches (in kb).
  *
  * Returns: The number of stats provided or -1 in case of failure.
  */
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 825b2b27e6..f88fb44373 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -19795,6 +19795,7 @@ qemuDomainGetStatsBalloon(virQEMUDriverPtr driver,
 STORE_MEM_RECORD(RSS, "rss")
 STORE_MEM_RECORD(LAST_UPDATE, "last-update")
 STORE_MEM_RECORD(USABLE, "usable")
+STORE_MEM_RECORD(DISK_CACHES, "disk_caches")
 }
 
 #undef STORE_MEM_RECORD
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 3e90279b71..9d161fe6f4 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -2071,6 +2071,8 @@ int qemuMonitorJSONGetMemoryStats(qemuMonitorPtr mon,
   VIR_DOMAIN_MEMORY_STAT_USABLE, 1024);
 GET_BALLOON_STATS(data, "last-update",
   VIR_DOMAIN_MEMORY_STAT_LAST_UPDATE, 1);
+GET_BALLOON_STATS(statsdata, "stat-disk-caches",
+  VIR_DOMAIN_MEMORY_STAT_DISK_CACHES, 1024);
 ret = got;
  cleanup:
 virJSONValueFree(cmd);
diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
index 87660ee674..b9b4f9739b 100644
--- a/tools/virsh-domain-monitor.c
+++ b/tools/virsh-domain-monitor.c
@@ -364,6 +364,8 @@ cmdDomMemStat(vshControl *ctl, const vshCmd *cmd)
 vshPrint(ctl, "rss %llu\n", stats[i].val);
 if (stats[i].tag == VIR_DOMAIN_MEMORY_STAT_LAST_UPDATE)
 vshPrint(ctl, "last_update %llu\n", stats[i].val);
+if (stats[i].tag == VIR_DOMAIN_MEMORY_STAT_DISK_CACHES)
+vshPrint(ctl, "disk_caches %llu\n", stats[i].val);
 }
 
 ret = true;
diff --git a/tools/virsh.pod b/tools/virsh.pod
index dc100db9f3..50799cf588 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -924,6 +924,8 @@ B:
   usable- The amount of memory which can be reclaimed by balloon
 without causing host swapping (in KiB)
   last-update   - Timestamp of the last update of statistics (in seconds)
+  disk_caches   - The amount of memory that can be reclaimed without
+additional I/O, typically disk caches (in KiB)
 
 For QEMU/KVM with a memory balloon, setting the optional I<--period> to a
 value larger than 0 in seconds will allow the balloon driver to return
@@ -1030,6 +1032,9 @@ I<--balloon> returns:
 balloon without causing host swapping (in KiB)
  "balloon.last-update" - timestamp of the last update of statistics
  (in seconds)
+ "balloon.disk_caches " - the amount of memory that can be reclaimed
+  without additional I/O, typically disk
+  caches (in KiB)
 
 I<--vcpu> returns:
 
-- 
2.17.1

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

[libvirt] [PATCH v3 0/1] qemu: Add entry for balloon stat stat-disk-caches

[Tomáš Golembiovský]

v3: changes suggested by John Ferlan
- updated commit message
- fixed units (bytes -> kB) in one comment
- NOTE: Various spelling is used throughout libvirt (kb/kB/KiB) interchangeably
  with the same meaning. I tried to keep the spelling consistent with the
  context around the place where the change occurred.
- updated qemuDomainGetStatsBalloon()
- moved the entry in virsh output to last position
- updated virsh.pod

v2:
- moved item to last position in array
- documented item also in libvirt-domain.c
- added item to virsh listing

Tomáš Golembiovský (1):
  qemu: Add entry for balloon stat stat-disk-caches

 include/libvirt/libvirt-domain.h | 9 -
 src/libvirt-domain.c | 3 +++
 src/qemu/qemu_driver.c   | 1 +
 src/qemu/qemu_monitor_json.c | 2 ++
 tools/virsh-domain-monitor.c | 2 ++
 tools/virsh.pod  | 5 +
 6 files changed, 21 insertions(+), 1 deletion(-)

-- 
2.17.1

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

[libvirt] [PATCH] util:Fix with process number and pid file do not match

[dubo163]

From: dubobo 

the libvirtd pid file is not match the os process pid number
which is smaller than before.

this would be exist if the libvirtd process coredump or the os
process was killed which the next pid number is smaller.

you can be also edit the pid file to write the longer number than
before,then restart the libvirtd service.

Signed-off-by: dubobo 
---
 src/util/virpidfile.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/src/util/virpidfile.c b/src/util/virpidfile.c
index 58ab29f..8b0ff99 100644
--- a/src/util/virpidfile.c
+++ b/src/util/virpidfile.c
@@ -445,6 +445,12 @@ int virPidFileAcquirePath(const char *path,
 }
 
 snprintf(pidstr, sizeof(pidstr), "%lld", (long long) pid);
+if (ftruncate(fd, 0) < 0) {
+VIR_FORCE_CLOSE(fd);
+return -1;
+}
+
+lseek(fd, 0, SEEK_SET);
 
 if (safewrite(fd, pidstr, strlen(pidstr)) < 0) {
 virReportSystemError(errno,
-- 
1.8.3.1

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

Re: [libvirt] [jenkins-ci PATCH] projects: libvirt-dbus uses xz now

[Pavel Hrdina]

On Mon, Jul 02, 2018 at 11:59:25AM +0200, Andrea Bolognani wrote:
> Since commit 28ad40b3d4ab, libvirt-dbus uses xz rather than
> gzip to compress release archives. Adapt to the change.
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  projects/libvirt-dbus.yaml | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Pavel Hrdina 


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

[libvirt] [jenkins-ci PATCH] projects: libvirt-dbus uses xz now

[Andrea Bolognani]

Since commit 28ad40b3d4ab, libvirt-dbus uses xz rather than
gzip to compress release archives. Adapt to the change.

Signed-off-by: Andrea Bolognani 
---
 projects/libvirt-dbus.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/projects/libvirt-dbus.yaml b/projects/libvirt-dbus.yaml
index c460db4..eaf5a61 100644
--- a/projects/libvirt-dbus.yaml
+++ b/projects/libvirt-dbus.yaml
@@ -2,6 +2,7 @@
 - project:
 name: libvirt-dbus
 title: Libvirt D-Bus
+archive_format: xz
 jobs:
   - autotools-build-job:
   parent_jobs: 'libvirt-glib-master-build'
-- 
2.17.1

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

[libvirt] [PATCH v3] qemu: format serial and geometry on frontend disk device

[Daniel P . Berrangé]

Currently we format the serial, geometry and error policy on the -drive
backend argument.

QEMU added the ability to set serial and geometry on the frontend in
the 1.2 release deprecating use of -drive, with support being deleted
from -drive in 3.0.

We keep formatting error policy on -drive for now, because we don't
ahve support for that with -device for usb-storage just yet.

Note that some disk buses (sd) still don't support -device. Although
QEMU allowed these properties to be set on -drive for if=sd, they
have been ignored so we now report an error in this case.

Signed-off-by: Daniel P. Berrangé 
---

Changed in v3:

 - Report error for setting CHS on USB
 - Report error for setting CHS translation mode for bus != IDE
 - Use 'bios-chs-trans' property not 'trans'

 src/qemu/qemu_command.c   | 46 ---
 .../disk-drive-network-tlsx509.args   | 12 ++---
 .../disk-drive-network-vxhs.args  |  4 +-
 tests/qemuxml2argvdata/disk-drive-shared.args |  5 +-
 tests/qemuxml2argvdata/disk-geometry.args |  6 +--
 tests/qemuxml2argvdata/disk-ide-wwn.args  |  5 +-
 .../qemuxml2argvdata/disk-scsi-disk-wwn.args  |  4 +-
 tests/qemuxml2argvdata/disk-serial.args   | 10 ++--
 tests/qemuxml2argvdata/disk-serial.xml|  1 -
 tests/qemuxml2xmloutdata/disk-serial.xml  |  1 -
 10 files changed, 62 insertions(+), 32 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 4fc3176ad3..46726b055e 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1599,7 +1599,7 @@ 
qemuBuildDiskFrontendAttributeErrorPolicy(virDomainDiskDefPtr disk,
 }
 
 
-static void
+static int
 qemuBuildDiskFrontendAttributes(virDomainDiskDefPtr disk,
 virBufferPtr buf)
 {
@@ -1607,14 +1607,27 @@ qemuBuildDiskFrontendAttributes(virDomainDiskDefPtr 
disk,
 if (disk->geometry.cylinders > 0 &&
 disk->geometry.heads > 0 &&
 disk->geometry.sectors > 0) {
+if (disk->bus == VIR_DOMAIN_DISK_BUS_USB) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("CHS geometry can not be set for 'usb' bus"));
+return -1;
+}
+
 virBufferAsprintf(buf, ",cyls=%u,heads=%u,secs=%u",
   disk->geometry.cylinders,
   disk->geometry.heads,
   disk->geometry.sectors);
 
-if (disk->geometry.trans != VIR_DOMAIN_DISK_TRANS_DEFAULT)
-virBufferAsprintf(buf, ",trans=%s",
+if (disk->geometry.trans != VIR_DOMAIN_DISK_TRANS_DEFAULT) {
+if (disk->bus != VIR_DOMAIN_DISK_BUS_IDE) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("CHS translation mode can only be set for 
'ide' bus not '%s'"),
+   virDomainDiskBusTypeToString(disk->bus));
+return -1;
+}
+virBufferAsprintf(buf, ",bios-chs-trans=%s",
   
virDomainDiskGeometryTransTypeToString(disk->geometry.trans));
+}
 }
 
 if (disk->serial) {
@@ -1622,7 +1635,7 @@ qemuBuildDiskFrontendAttributes(virDomainDiskDefPtr disk,
 virBufferEscape(buf, '\\', " ", "%s", disk->serial);
 }
 
-qemuBuildDiskFrontendAttributeErrorPolicy(disk, buf);
+return 0;
 }
 
 
@@ -1662,11 +1675,27 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
 virBufferAsprintf(, "if=%s",
   virDomainDiskQEMUBusTypeToString(disk->bus));
 virBufferAsprintf(, ",index=%d", idx);
+
+if (disk->serial) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Serial property not supported for drive bus 
'%s'"),
+   virDomainDiskBusTypeToString(disk->bus));
+goto error;
+}
+if (disk->geometry.cylinders > 0 &&
+disk->geometry.heads > 0 &&
+disk->geometry.sectors > 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Geometry not supported for drive bus '%s'"),
+   virDomainDiskBusTypeToString(disk->bus));
+goto error;
+}
 }
 
-/* Format attributes for the drive itself (not the storage backing it) 
which
- * we've formatted historically with -drive */
-qemuBuildDiskFrontendAttributes(disk, );
+/* werror/rerror are really frontend attributes, but older
+ * qemu requires them on -drive instead of -device */
+qemuBuildDiskFrontendAttributeErrorPolicy(disk, );
+
 
 /* While this is a frontend attribute, it only makes sense to be used when
  * legacy -drive is used. In modern qemu the 'ide-cd' or 'scsi-cd' are 
used.
@@ -2125,6 +2154,9 @@ qemuBuildDriveDevStr(const virDomainDef *def,
 if (qemuBuildDriveDevCacheStr(disk, , qemuCaps) < 0)
 goto error;
 
+if (qemuBuildDiskFrontendAttributes(disk, ) < 0)

Re: [libvirt] [PATCH v2] qemu: format serial and geometry on frontend disk device

[Daniel P . Berrangé]

On Mon, Jul 02, 2018 at 10:49:09AM +0200, Peter Krempa wrote:
> On Mon, Jul 02, 2018 at 09:26:38 +0100, Daniel Berrange wrote:
> > Currently we format the serial, geometry and error policy on the -drive
> > backend argument.
> > 
> > QEMU added the ability to set serial and geometry on the frontend in
> > the 1.2 release deprecating use of -drive, with support being deleted
> > from -drive in 3.0.
> > 
> > We keep formatting error policy on -drive for now, because we don't
> > ahve support for that with -device for usb-storage just yet.
> > 
> > Note that some disk buses (sd) still don't support -device. Although
> > QEMU allowed these properties to be set on -drive for if=sd, they
> > have been ignored so we now report an error in this case.
> > 
> > Signed-off-by: Daniel P. Berrangé 
> > ---
> >  src/qemu/qemu_command.c   | 26 +++
> >  .../disk-drive-network-tlsx509.args   | 12 -
> >  .../disk-drive-network-vxhs.args  |  4 +--
> >  tests/qemuxml2argvdata/disk-drive-shared.args |  5 ++--
> >  tests/qemuxml2argvdata/disk-geometry.args |  6 ++---
> >  tests/qemuxml2argvdata/disk-ide-wwn.args  |  5 ++--
> >  .../qemuxml2argvdata/disk-scsi-disk-wwn.args  |  4 +--
> >  tests/qemuxml2argvdata/disk-serial.args   | 10 +++
> >  tests/qemuxml2argvdata/disk-serial.xml|  1 -
> >  tests/qemuxml2xmloutdata/disk-serial.xml  |  1 -
> >  10 files changed, 44 insertions(+), 30 deletions(-)
> 
> ACK

Damn self-NACK

The 'trans' property was renamed to 'bios-chs-trans', and is only valid on
IDE, not SCSI/virtio

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 v2] qemu: format serial and geometry on frontend disk device

[Peter Krempa]

On Mon, Jul 02, 2018 at 09:26:38 +0100, Daniel Berrange wrote:
> Currently we format the serial, geometry and error policy on the -drive
> backend argument.
> 
> QEMU added the ability to set serial and geometry on the frontend in
> the 1.2 release deprecating use of -drive, with support being deleted
> from -drive in 3.0.
> 
> We keep formatting error policy on -drive for now, because we don't
> ahve support for that with -device for usb-storage just yet.
> 
> Note that some disk buses (sd) still don't support -device. Although
> QEMU allowed these properties to be set on -drive for if=sd, they
> have been ignored so we now report an error in this case.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/qemu/qemu_command.c   | 26 +++
>  .../disk-drive-network-tlsx509.args   | 12 -
>  .../disk-drive-network-vxhs.args  |  4 +--
>  tests/qemuxml2argvdata/disk-drive-shared.args |  5 ++--
>  tests/qemuxml2argvdata/disk-geometry.args |  6 ++---
>  tests/qemuxml2argvdata/disk-ide-wwn.args  |  5 ++--
>  .../qemuxml2argvdata/disk-scsi-disk-wwn.args  |  4 +--
>  tests/qemuxml2argvdata/disk-serial.args   | 10 +++
>  tests/qemuxml2argvdata/disk-serial.xml|  1 -
>  tests/qemuxml2xmloutdata/disk-serial.xml  |  1 -
>  10 files changed, 44 insertions(+), 30 deletions(-)

ACK


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

Re: [libvirt] [PATCH 2/2] qemu_migration: Check for active domain after talking to remote daemon

[Peter Krempa]

On Thu, Jun 28, 2018 at 16:06:47 +0200, Jiri Denemark wrote:
> Once we called qemuDomainObjEnterRemote to talk to the destination
> daemon during a peer to peer migration, the vm lock is released and we
> only hold an async job. If the source domain dies at this point the
> monitor EOF callback is allowed to do its job and (among other things)
> clear all private data irrelevant for stopped domain. Thus when we call
> qemuDomainObjExitRemote, the domain may already be gone and we should
> avoid touching runtime private data (such as current job info).
> 
> In other words after acquiring the lock in qemuDomainObjExitRemote, we
> need to check the domain is still alive. Unless we're doing offline
> migration.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1589730
> 
> Signed-off-by: Jiri Denemark 
> ---
>  src/qemu/qemu_domain.c| 14 +++-
>  src/qemu/qemu_domain.h|  5 +++--
>  src/qemu/qemu_migration.c | 45 ++-
>  3 files changed, 41 insertions(+), 23 deletions(-)

[...]

> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index c9aaa38029..cafab2f3e8 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -3920,13 +3920,15 @@ qemuMigrationSrcPerformPeer2Peer2(virQEMUDriverPtr 
> driver,
>  qemuDomainObjEnterRemote(vm);
>  ret = dconn->driver->domainMigratePrepareTunnel
>  (dconn, st, destflags, dname, resource, dom_xml);
> -qemuDomainObjExitRemote(vm);
> +if (qemuDomainObjExitRemote(vm, true) < 0)
> +goto cleanup;
>  } else {
>  qemuDomainObjEnterRemote(vm);
>  ret = dconn->driver->domainMigratePrepare2
>  (dconn, , , NULL, _out,
>   destflags, dname, resource, dom_xml);
> -qemuDomainObjExitRemote(vm);
> +if (qemuDomainObjExitRemote(vm, true) < 0)
> +goto cleanup;
>  }
>  VIR_FREE(dom_xml);
>  if (ret == -1)

There is a check below this which does !virDomainObjIsActive which seems
pointless now.


> @@ -3987,7 +3989,8 @@ qemuMigrationSrcPerformPeer2Peer2(virQEMUDriverPtr 
> driver,
>  ddomain = dconn->driver->domainMigrateFinish2
>  (dconn, dname, cookie, cookielen,
>   uri_out ? uri_out : dconnuri, destflags, cancelled);
> -qemuDomainObjExitRemote(vm);
> +/* The domain is already gone at this point */
> +ignore_value(qemuDomainObjExitRemote(vm, false));
>  if (cancelled && ddomain)
>  VIR_ERROR(_("finish step ignored that migration was cancelled"));
>  

[...]

> @@ -4488,20 +4500,13 @@ qemuMigrationSrcPerformPeer2Peer(virQEMUDriverPtr 
> driver,
>  goto cleanup;
>  }
>  
> -if (flags & VIR_MIGRATE_OFFLINE && !dstOffline) {
> +if (offline && !dstOffline) {

This slightly mixes two distinct refactors ...

>  virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
> _("offline migration is not supported by "
>   "the destination host"));
>  goto cleanup;
>  }

ACK


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

[libvirt] [dbus PATCH] Fix 'make rpm'

[Andrea Bolognani]

Commit 28ad40b3d4ab changed the compression algorithm for
release archives from gzip to xz, but the 'rpm' target has
not been updated accordingly.

Signed-off-by: Andrea Bolognani 
---
Pushed under the (pre-emptive) build breaker rule.

 Makefile.am | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile.am b/Makefile.am
index 93941c6..8abba68 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -20,7 +20,7 @@ DISTCLEAN_FILES = \
$(NULL)
 
 rpm: clean
-   @(unset CDPATH ; $(MAKE) dist && rpmbuild -ta $(distdir).tar.gz)
+   @(unset CDPATH ; $(MAKE) dist && rpmbuild -ta $(distdir).tar.xz)
 
 dist-hook: gen-AUTHORS
 
-- 
2.17.1

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

[libvirt] [PATCH v2] qemu: format serial and geometry on frontend disk device

[Daniel P . Berrangé]

Currently we format the serial, geometry and error policy on the -drive
backend argument.

QEMU added the ability to set serial and geometry on the frontend in
the 1.2 release deprecating use of -drive, with support being deleted
from -drive in 3.0.

We keep formatting error policy on -drive for now, because we don't
ahve support for that with -device for usb-storage just yet.

Note that some disk buses (sd) still don't support -device. Although
QEMU allowed these properties to be set on -drive for if=sd, they
have been ignored so we now report an error in this case.

Signed-off-by: Daniel P. Berrangé 
---
 src/qemu/qemu_command.c   | 26 +++
 .../disk-drive-network-tlsx509.args   | 12 -
 .../disk-drive-network-vxhs.args  |  4 +--
 tests/qemuxml2argvdata/disk-drive-shared.args |  5 ++--
 tests/qemuxml2argvdata/disk-geometry.args |  6 ++---
 tests/qemuxml2argvdata/disk-ide-wwn.args  |  5 ++--
 .../qemuxml2argvdata/disk-scsi-disk-wwn.args  |  4 +--
 tests/qemuxml2argvdata/disk-serial.args   | 10 +++
 tests/qemuxml2argvdata/disk-serial.xml|  1 -
 tests/qemuxml2xmloutdata/disk-serial.xml  |  1 -
 10 files changed, 44 insertions(+), 30 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 4fc3176ad3..86d4b10f74 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1621,8 +1621,6 @@ qemuBuildDiskFrontendAttributes(virDomainDiskDefPtr disk,
 virBufferAddLit(buf, ",serial=");
 virBufferEscape(buf, '\\', " ", "%s", disk->serial);
 }
-
-qemuBuildDiskFrontendAttributeErrorPolicy(disk, buf);
 }
 
 
@@ -1662,11 +1660,27 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
 virBufferAsprintf(, "if=%s",
   virDomainDiskQEMUBusTypeToString(disk->bus));
 virBufferAsprintf(, ",index=%d", idx);
+
+if (disk->serial) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Serial property not supported for drive bus 
'%s'"),
+   virDomainDiskBusTypeToString(disk->bus));
+goto error;
+}
+if (disk->geometry.cylinders > 0 &&
+disk->geometry.heads > 0 &&
+disk->geometry.sectors > 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Geometry not supported for drive bus '%s'"),
+   virDomainDiskBusTypeToString(disk->bus));
+goto error;
+}
 }
 
-/* Format attributes for the drive itself (not the storage backing it) 
which
- * we've formatted historically with -drive */
-qemuBuildDiskFrontendAttributes(disk, );
+/* werror/rerror are really frontend attributes, but older
+ * qemu requires them on -drive instead of -device */
+qemuBuildDiskFrontendAttributeErrorPolicy(disk, );
+
 
 /* While this is a frontend attribute, it only makes sense to be used when
  * legacy -drive is used. In modern qemu the 'ide-cd' or 'scsi-cd' are 
used.
@@ -2125,6 +2139,8 @@ qemuBuildDriveDevStr(const virDomainDef *def,
 if (qemuBuildDriveDevCacheStr(disk, , qemuCaps) < 0)
 goto error;
 
+qemuBuildDiskFrontendAttributes(disk, );
+
 if (virBufferCheckError() < 0)
 goto error;
 
diff --git a/tests/qemuxml2argvdata/disk-drive-network-tlsx509.args 
b/tests/qemuxml2argvdata/disk-drive-network-tlsx509.args
index e25f45742c..b5e73064c0 100644
--- a/tests/qemuxml2argvdata/disk-drive-network-tlsx509.args
+++ b/tests/qemuxml2argvdata/disk-drive-network-tlsx509.args
@@ -28,22 +28,22 @@ server,nowait \
 -drive file.driver=vxhs,file.tls-creds=objvirtio-disk0_tls0,\
 file.vdisk-id=eb90327c-8302-4725-9e1b-4e85ed4dc251,\
 file.server.host=192.168.0.1,file.server.port=,format=raw,if=none,\
-id=drive-virtio-disk0,serial=eb90327c-8302-4725-9e1b-4e85ed4dc251,cache=none \
+id=drive-virtio-disk0,cache=none \
 -device virtio-blk-pci,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,\
-id=virtio-disk0 \
+id=virtio-disk0,serial=eb90327c-8302-4725-9e1b-4e85ed4dc251 \
 -object 
tls-creds-x509,id=objvirtio-disk1_tls0,dir=/etc/pki/libvirt-vxhs/dummy,\
 ,path,endpoint=client,verify-peer=yes \
 -drive file.driver=vxhs,file.tls-creds=objvirtio-disk1_tls0,\
 file.vdisk-id=eb90327c-8302-4725-9e1b-4e85ed4dc252,\
 file.server.host=192.168.0.2,file.server.port=,format=raw,if=none,\
-id=drive-virtio-disk1,serial=eb90327c-8302-4725-9e1b-4e85ed4dc252,cache=none \
+id=drive-virtio-disk1,cache=none \
 -device virtio-blk-pci,bus=pci.0,addr=0x5,drive=drive-virtio-disk1,\
-id=virtio-disk1 \
+id=virtio-disk1,serial=eb90327c-8302-4725-9e1b-4e85ed4dc252 \
 -drive file.driver=vxhs,file.vdisk-id=eb90327c-8302-4725-9e1b-4e85ed4dc253,\
 file.server.host=192.168.0.3,file.server.port=,format=raw,if=none,\
-id=drive-virtio-disk2,serial=eb90327c-8302-4725-9e1b-4e85ed4dc252,cache=none \
+id=drive-virtio-disk2,cache=none \
 -device 

Re: [libvirt] [PATCH v3 2/3] qemu: Implement the HTM pSeries feature

[Andrea Bolognani]

On Sat, 2018-06-30 at 07:23 -0400, John Ferlan wrote:
> On 06/26/2018 06:21 AM, Andrea Bolognani wrote:
> > https://bugzilla.redhat.com/show_bug.cgi?id=1525599
> > Signed-off-by: Andrea Bolognani 
> > ---
> >  docs/formatdomain.html.in |  8 
> >  docs/schemas/domaincommon.rng |  5 +
> >  src/conf/domain_conf.c| 19 ++
> >  src/conf/domain_conf.h|  1 +
> >  src/qemu/qemu_command.c   | 20 +++
> >  src/qemu/qemu_domain.c| 13 
> >  tests/qemuxml2argvdata/pseries-features.args  |  2 +-
> >  tests/qemuxml2argvdata/pseries-features.xml   |  1 +
> >  tests/qemuxml2argvtest.c  |  1 +
> >  tests/qemuxml2xmloutdata/pseries-features.xml |  1 +
> >  tests/qemuxml2xmltest.c   |  1 +
> >  11 files changed, 71 insertions(+), 1 deletion(-)
> 
> Even though it's QEMU/KVM only - the conf/docs/xml2xml should be
> separated from the qemu/xml2argv changes.

Can do.

> > @@ -2162,6 +2163,13 @@
> >Enable QEMU vmcoreinfo device to let the guest kernel save debug
> >details. Since 4.4.0 (QEMU only)
> >
> > +  htm
> > +  Configure HTM (Hardware Transational Memory) availability for
> > +  pSeries guests. Possible values for the state 
> > attribute
> > +  are on and off. If the attribute is not
> > +  defined, the hypervisor default will be used.
> > +  Since 4.5.0 (QEMU/KVM only)
> 
> This'll miss 4.5, so change to 4.6 before pushing

Sure.

> > +case VIR_DOMAIN_FEATURE_HTM:
> > +if (!(tmp = virXMLPropString(nodes[i], "state"))) {
> > +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > +   _("missing state attribute '%s' of feature 
> > '%s'"),
> > +   tmp, virDomainFeatureTypeToString(val));
> > +goto error;
> > +}
> > +if ((def->features[val] = 
> > virTristateSwitchTypeFromString(tmp)) < 0) {
> 
> [1] checked here, so that...
> 
> > +if (def->features[VIR_DOMAIN_FEATURE_HTM] != 
> > VIR_TRISTATE_SWITCH_ABSENT) {
> > +const char *str;
> > +
> > +if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_PSERIES_CAP_HTM)) {
> > +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > +   _("HTM configuration is not supported by this "
> > + "QEMU binary"));
> > +goto cleanup;
> > +}
> > +
> > +str = 
> > virTristateSwitchTypeToString(def->features[VIR_DOMAIN_FEATURE_HTM]);
> > +if (!str) {
> 
> [1] ... this is unnecessary due to virDomainDefParseXML check.

I strongly dislike the practice of skipping checks in a function
with the rationale that they're already performed somewhere else in
the code base: libvirt is in a state of constant flux, and as stuff
gets moved around you might very well find yourself no longer as
shielded from invalid values as you thought you were. Local sanity
checks should IMHO always be performed locally.

tl;dr I'd really rather keep this in.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH 1/2] qemu_migration: Rename 'offline' variable in SrcPerformPeer2Peer

[Peter Krempa]

On Thu, Jun 28, 2018 at 16:06:46 +0200, Jiri Denemark wrote:
> The variable is used to store the offline migration capability of the
> destination daemon. Let's call it 'dstOffline' so that we can later use
> 'offline' to indicate whether we were asked to do offline migration.
> 
> Signed-off-by: Jiri Denemark 
> ---
>  src/qemu/qemu_migration.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)

ACK


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

Re: [libvirt] [dbus PATCH 00/15] Build system fixes, cleanups and tweaks

[Pavel Hrdina]

On Fri, Jun 29, 2018 at 06:01:07PM +0200, Andrea Bolognani wrote:
> It all started when I got
> 
>   tests/Makefile.am:17: warning: source file '$(top_srcdir)/src/util.c' is in 
> a subdirectory,
>   tests/Makefile.am:17: but option 'subdir-objects' is disabled
>   automake: warning: possible forward-incompatibility.
>   automake: At least a source file is in a subdirectory, but the 
> 'subdir-objects'
>   automake: automake option hasn't been enabled.  For now, the corresponding 
> output
>   automake: object file(s) will be placed in the top-level directory.  
> However,
>   automake: this behaviour will change in future Automake versions: they will
>   automake: unconditionally cause object files to be placed in the same 
> subdirectory
>   automake: of the corresponding sources.
>   automake: You are advised to start using 'subdir-objects' option throughout 
> your
>   automake: project, to avoid future incompatibilities.
> 
> when running autogen.sh and decided to investigate; before
> I realized it, I was already deep down the rabbit hole and
> falling towards Wonderland at an increasingly concerning
> speed.
> 
> Andrea Bolognani (15):
>   gitignore: Fix man page pattern
>   gitignore: Fix aclocal.m4 pattern
>   tests: Don't distribute compiled binaries
>   src: Fix typo in PIE_LDFLAGS
>   src: Remove empty CLEANFILES
>   src: Don't list source files in EXTRA_DIST
>   tests: Move includes to AM_CPPFLAGS
>   src: Make CFLAGS and LDFLAGS global
>   configure: Enable libtool
>   src: Build libutil.la separately
>   configure: Enable subdir-objects
>   configure: Enable -Wno-obsolete and tar-pax
>   configure: Use xz for release archives
>   data: Rename some variables to *_in_files
>   autotools: Use consistent style

Reviewed-by: Pavel Hrdina 


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

Re: [libvirt] [PATCH v3 1/3] qemu: Add capability for the HTM pSeries feature

[Andrea Bolognani]

On Sat, 2018-06-30 at 07:23 -0400, John Ferlan wrote:
> Weird to see having htm supported after the max page size - chicken and
> egg type problem.  I mean how do you have hpt without having max page
> size.  Anyway, just one of life's ironies I suppose. I would think just
> having page size is enough, but what do I really know about pSeries, not
> much ;-). If that's really not the case, well then fine separate caps it is.

Note that this is about HTM, not HPT - that's a completely
different TLA! The two features are not related in any way.

> BTW: Although it is a bz, it's "debatable" if you're fixing a bug or
> enabling a feature during freeze even though it was posted before
> freeze, so this I believe should wait for 4.6 to open.

Sure, I would never have pushed this during the freeze :)

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [Qemu-devel] [PULL 25/26] block: Remove deprecated -drive option serial

[Kevin Wolf]

Am 25.06.2018 um 13:45 hat Peter Krempa geschrieben:
> On Mon, Jun 25, 2018 at 13:41:06 +0200, Kevin Wolf wrote:
> > Am 25.06.2018 um 11:53 hat Daniel P. Berrangé geschrieben:
> > > On Fri, Jun 22, 2018 at 03:31:46PM +0100, Daniel P. Berrangé wrote:
> > > > On Fri, Jun 22, 2018 at 04:25:13PM +0200, Kevin Wolf wrote:
> > > > > Am 22.06.2018 um 15:36 hat Christian Borntraeger geschrieben:
> 
> [...]
> 
> > 
> > Thanks!
> > 
> > I'll look into werror/rerror support for usb-storage. It shouldn't be
> > too hard, though it's strictly speaking a separate problem related to
> > using -blockdev rather than option deprecation.
> > 
> > If Peter wants to wait for QEMU support before converting werror/rerror
> 
> Definitely. I don't want to keep around yet another hack that will
> satisfy one specific case and then add another capability for it. We
> should then gate the moving of the feature based on the presence of
> werror for usb-storage.
> 
> > to -device, maybe it would make sense to split your patch for v2 so that
> > geometry and serial can get fixed right away?
> 
> Yes this can be done right away.

Has serial/gemoetry been fixed meanwhile and will it make it into the
next release?

Kevin


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

Re: [libvirt] [PATCH 3/5] virStorageBackendIQNFound: Rework iscsiadm output parsing

[Peter Krempa]

On Fri, Jun 29, 2018 at 17:01:49 +0200, Michal Privoznik wrote:
> Firstly, we can utilize virCommandSetOutputBuffer() API which
> will collect the command output for us. Secondly, sscanf()-ing
> through each line is easier to understand (and more robust) than
> jumping over a string with strchr().
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/util/viriscsi.c | 85 
> +
>  1 file changed, 34 insertions(+), 51 deletions(-)
> 
> diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c
> index 2e55b3c10b..44788056fd 100644
> --- a/src/util/viriscsi.c
> +++ b/src/util/viriscsi.c

[...]

> @@ -117,71 +116,56 @@ static int
>  virStorageBackendIQNFound(const char *initiatoriqn,
>char **ifacename)
>  {
> -int ret = IQN_ERROR, fd = -1;
> -char ebuf[64];
> -FILE *fp = NULL;
> -char *line = NULL, *newline = NULL, *iqn = NULL, *token = NULL;
> +int ret = IQN_ERROR;
> +char *outbuf = NULL;
> +char *line = NULL;
> +char *iface = NULL;
> +char *iqn = NULL;
>  virCommandPtr cmd = virCommandNewArgList(ISCSIADM,
>   "--mode", "iface", NULL);
>  
>  *ifacename = NULL;
>  
> -if (VIR_ALLOC_N(line, LINE_SIZE) != 0) {
> -virReportError(VIR_ERR_INTERNAL_ERROR,
> -   _("Could not allocate memory for output of '%s'"),
> -   ISCSIADM);
> +virCommandSetOutputBuffer(cmd, );
> +if (virCommandRun(cmd, NULL) < 0)
>  goto cleanup;
> -}
>  
> -memset(line, 0, LINE_SIZE);
> +/* Example of data we are dealing with:
> + * default tcp
> + * iser iser
> + * libvirt-iface-253db048 
> tcpiqn.2017-03.com.user:client
> + */
>  
> -virCommandSetOutputFD(cmd, );
> -if (virCommandRunAsync(cmd, NULL) < 0)
> -goto cleanup;
> +line = outbuf;
> +while (line) {

This is severely misleading and makes this loop technically infinite. Or
just checks that output buffer was not null. Both operations should be
made explicit. Or you meant *line or line[0]

> +char *newline;
> +int num;
>  
> -if ((fp = VIR_FDOPEN(fd, "r")) == NULL) {
> -virReportError(VIR_ERR_INTERNAL_ERROR,
> -   _("Failed to open stream for file descriptor "
> - "when reading output from '%s': '%s'"),
> -   ISCSIADM, virStrerror(errno, ebuf, sizeof(ebuf)));
> -goto cleanup;
> -}
> +if (!(newline = strchr(line, '\n')))
> +break;

This is ther reason why it's working at this point.

>  
> -while (fgets(line, LINE_SIZE, fp) != NULL) {
> -newline = strrchr(line, '\n');


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

Re: [libvirt] [dbus PATCH 09/15] configure: Enable libtool

[Andrea Bolognani]

On Fri, 2018-06-29 at 20:15 +0200, Pavel Hrdina wrote:
> On Fri, Jun 29, 2018 at 06:01:16PM +0200, Andrea Bolognani wrote:
> > We're going to start using libtool convenience libraries
> > in a second.
>
> We should also update spec file to include:
> 
> BuildRequires: libtool

Fair enough :)

I'll squash that in.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH] qemuDomainDeviceDefValidateNetwork: Check for range only if IP prefix set

[Peter Krempa]

On Fri, Jun 29, 2018 at 16:56:12 +0200, Michal Privoznik wrote:
> The @prefix attribute to  element for interface type user is
> optional. Therefore, if left out it has value of zero in which
> case we should not check whether it falls into <4, 27> range.
> Otherwise we fail parsing domain XML for no good reason.
> 
> Signed-off-by: Michal Privoznik 

Broken in commit b62b8090b2ad4524a5bf9d40d0d1c17a9d57f5a0

also since this patch originates from the bugzilla mentioned in the
above commit you should add it here as well.

> ---
>  src/qemu/qemu_domain.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index afd572fc5e..c1798edf41 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -4373,7 +4373,8 @@ qemuDomainDeviceDefValidateNetwork(const 
> virDomainNetDef *net)
>  }
>  hasIPv4 = true;
>  
> -if (ip->prefix < 4 || ip->prefix > 27) {
> +if (ip->prefix > 0 &&
> +(ip->prefix < 4 || ip->prefix > 27)) {
>  virReportError(VIR_ERR_XML_ERROR, "%s",
> _("invalid prefix, must be in range of 
> 4-27"));
>  return -1;

ACK with the commit message ammended.


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

Re: [libvirt] [PATCH 1/3] util: new function virNetDevOpenvswitchInterfaceGetMaster()

[Michal Prívozník]

On 07/02/2018 01:46 AM, Laine Stump wrote:
> This function retrieves the name of the OVS bridge that the given
> netdev is attached to. This separate function is necessary because OVS
> set the IFLA_MASTER attribute to "ovs-system" for all netdevs that are
> attached to an OVS bridge, so the standard method of retrieving the
> master can't be used.
> 
> Signed-off-by: Laine Stump 
> ---
>  src/libvirt_private.syms|  1 +
>  src/util/virnetdevopenvswitch.c | 55 
> +
>  src/util/virnetdevopenvswitch.h |  6 +
>  3 files changed, 62 insertions(+)
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 98913a577a..386f53eeb9 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2364,6 +2364,7 @@ virNetDevMidonetUnbindPort;
>  virNetDevOpenvswitchAddPort;
>  virNetDevOpenvswitchGetMigrateData;
>  virNetDevOpenvswitchGetVhostuserIfname;
> +virNetDevOpenvswitchInterfaceGetMaster;
>  virNetDevOpenvswitchInterfaceStats;
>  virNetDevOpenvswitchRemovePort;
>  virNetDevOpenvswitchSetMigrateData;
> diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c
> index f86f698430..af3f2a773d 100644
> --- a/src/util/virnetdevopenvswitch.c
> +++ b/src/util/virnetdevopenvswitch.c
> @@ -404,6 +404,61 @@ virNetDevOpenvswitchInterfaceStats(const char *ifname,
>  return ret;
>  }
>  
> +
> +/**
> + * virNetDeOpenvswitchGetMaster:
> + * @ifname: name of interface we're interested in
> + * @master: used to return a string containing the name of @ifname's "master"
> + *  (this is the bridge or bond device that this device is attached 
> to)
> + *
> + * Returns 0 on success, -1 on failure (if @ifname has no master
> + * @master will be NULL, but return value will still be 0 (success)).
> + *
> + * NB: This function is needed because the IFLA_MASTER attribute of an
> + * interface in a netlink dump (see virNetDevGetMaster()) will always
> + * return "ovs-system" for any interface that is attached to an OVS
> + * switch. When that happens, virNetDevOpenvswitchInterfaceGetMaster()
> + * must be called to get the "real" master of the interface.
> + */
> +int
> +virNetDevOpenvswitchInterfaceGetMaster(const char *ifname, char **master)
> +{
> +virCommandPtr cmd = NULL;
> +int ret = -1;
> +int exitstatus;
> +
> +*master = NULL;
> +cmd = virCommandNew(OVSVSCTL);

I'd put an empty line in between these two ^^ because from data POV we
have two different blocks (one initializes variables, the other
constructs @cmd). But it is really a nit pick.

> +virNetDevOpenvswitchAddTimeout(cmd);
> +virCommandAddArgList(cmd, "iface-to-br", ifname, NULL);
> +virCommandSetOutputBuffer(cmd, master);
> +
> +if (virCommandRun(cmd, ) < 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("Unable to run command to get OVS master for "
> + "interface %s"), ifname);
> +goto cleanup;
> +}
> +
> +/* non-0 exit code just means that the interface has no master in OVS */
> +if (exitstatus != 0)
> +VIR_FREE(*master);

A-ha! We indeed need to do this. I was under impression that just like
other functions of ours if they fail no allocation is done. But this is
different so we need to call VIR_FREE().


Michal

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

Re: [libvirt] [PATCH 0/3] fix bad error log on libvirtd restart when using OVS

[Michal Prívozník]

On 07/02/2018 01:46 AM, Laine Stump wrote:
> This BZ:
> 
>   https://bugzilla.redhat.com/show_bug.cgi?id=1596176
> 
> and the log message of patch 3 explain the problem.
> 
> Laine Stump (3):
>   util: new function virNetDevOpenvswitchInterfaceGetMaster()
>   util: add some debug log to virNetDevGetMaster
>   network: properly check for taps that are connected to an OVS bridge
> 
>  src/libvirt_private.syms|  1 +
>  src/network/bridge_driver.c | 21 ++--
>  src/util/virnetdev.c|  1 +
>  src/util/virnetdevopenvswitch.c | 55 
> +
>  src/util/virnetdevopenvswitch.h |  6 +
>  5 files changed, 82 insertions(+), 2 deletions(-)
> 

ACK series.

Michal

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