Re: [libvirt] [PATCH v2 0/2] tests: Add nodeinfo test data utility scripts

[Andrea Bolognani]

On Wed, 2015-10-21 at 17:43 -0400, John Ferlan wrote:
> > Andrea Bolognani (2):
> >   tests: Add script to display nodeinfo test data
> >   tests: Add script to copy nodeinfo test data from host
> > 
> >  tests/nodeinfodata/copy-from-host.sh | 113
> > +++
> >  tests/nodeinfodata/display.sh| 113
> > +++
> >  2 files changed, 226 insertions(+)
> >  create mode 100755 tests/nodeinfodata/copy-from-host.sh
> >  create mode 100755 tests/nodeinfodata/display.sh
> 
> I'm ambivalent on this pair.
> 
> Not sure what the value of patch 1 is?  What should I expect to see
> given the arguments?  What does "ppc64_cpu --info" show?  Perhaps the
> better question is - if you run on each directory in nodeinfodata do
> you
> get what you expect?

I've run the script on every existing dataset and the output
was correct, as far as I can tell.

The script was immensely useful to me back when I was
implementing changes to the way the nodeinfo code counts
CPUs when subcorese are involved, eg.

  $ ./display.sh linux-subcores3 8
  Threads per core: 8
  Present CPUs: 0-159

  Core   0:0 1 2 3 4 5 6 7 
  Core   1:8*9101112131415 
  Core   2:   1617181920212223 
  Core   3:   2425262728293031 
  Core   4:   3233343536373839 
  Core   5:   40*   41424344454647 
  Core   6:   48*   49505152535455 
  Core   7:   56*   57585960616263 
  Core   8:   6465666768*   697071 
  Core   9:   72*   73747576777879 
  Core  10:   80*   81828384858687 
  Core  11:   88*   89909192939495 
  Core  12:   96*   979899   100   101   102   103 
  Core  13:  104*  105   106   107   108   109   110   111 
  Core  14:  112*  113   114   115   116   117   118   119 
  Core  15:  120   121   122   123   124   125   126   127 
  Core  16:  128*  129   130   131   132   133   134   135 
  Core  17:  136*  137   138   139   140   141   142   143 
  Core  18:  144   145   146   147   148   149   150   151 
  Core  19:  152*  153*  154*  155*  156*  157*  158*  159*

You can see at a glance there's something wrong with this
configuration - why is CPU 68 online? What about the last
line? This kind of report is especially useful when dealing
with processors with a high number of CPUs.

> As for patch 2, one would have to know they should use the
> copy-from-host.sh script. Perhaps what might be better and/or
> somewhat
> more interesting on this one is some make check rule that scans the
> nodeinfodata trees looking for files that shouldn't be there. That
> way
> if someone does use their own methodology to copy over the tree we'd
> know it (and could message to use the copy-from-host.sh script...

I agree, as it stands it's not very discoverable, plus
adding the check you suggest would also prevent something
like e739d95 from ever being needed again.

I'll work on that as soon as I have some time.

Cheers.

-- 
Andrea Bolognani
Software Engineer - Virtualization Team

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

Re: [libvirt] [PATCH] tests: Remove unused nodeinfo test data

[Andrea Bolognani]

On Wed, 2015-10-21 at 17:30 -0400, John Ferlan wrote:
> 
> On 10/07/2015 09:29 AM, Andrea Bolognani wrote:
> > A bunch of files that we don't currently parse, and are very
> > unlikely to ever start parsing, made their way into the nodeinfo
> > test data. Get rid of them.
> > ---
> >  tests/nodeinfodata/linux-f21-mustang/cpu/modalias 
> >| 1 -
> >  tests/nodeinfodata/linux-raspberrypi/cpu/cpuidle/current_driver   
> >| 1 -
> >  tests/nodeinfodata/linux
> > -raspberrypi/cpu/cpuidle/current_governor_ro | 1 -
> >  tests/nodeinfodata/linux-rhelsa-3.19.0-mustang/cpu/modalias   
> >| 1 -
> >  tests/nodeinfodata/linux-test8/cpu/cpuidle/current_driver 
> >| 1 -
> >  tests/nodeinfodata/linux-test8/cpu/cpuidle/current_governor_ro
> >| 1 -
> >  tests/nodeinfodata/linux-test8/cpu/sched_mc_power_savings 
> >| 1 -
> >  tests/nodeinfodata/linux-test8/cpu/sched_smt_power_savings
> >| 1 -
> >  8 files changed, 8 deletions(-)
> >  delete mode 100644 tests/nodeinfodata/linux-f21
> > -mustang/cpu/modalias
> >  delete mode 100644 tests/nodeinfodata/linux
> > -raspberrypi/cpu/cpuidle/current_driver
> >  delete mode 100644 tests/nodeinfodata/linux
> > -raspberrypi/cpu/cpuidle/current_governor_ro
> >  delete mode 100644 tests/nodeinfodata/linux-rhelsa-3.19.0
> > -mustang/cpu/modalias
> >  delete mode 100644 tests/nodeinfodata/linux
> > -test8/cpu/cpuidle/current_driver
> >  delete mode 100644 tests/nodeinfodata/linux
> > -test8/cpu/cpuidle/current_governor_ro
> >  delete mode 100644 tests/nodeinfodata/linux
> > -test8/cpu/sched_mc_power_savings
> >  delete mode 100644 tests/nodeinfodata/linux
> > -test8/cpu/sched_smt_power_savings
> > 
> 
> ACK - seems reasonable.

Pushed, thanks.

-- 
Andrea Bolognani
Software Engineer - Virtualization Team

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

Re: [libvirt] [PATCHv2] virsh: fix no error when pass a count <= 0 to setvcpus

[lhuang]

On 10/22/2015 03:32 PM, Andrea Bolognani wrote:

On Thu, 2015-10-22 at 11:27 +0800, Luyao Huang wrote:

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

When count <= 0, the client exit without set an error.

Signed-off-by: Luyao Huang 
---
v2:
- use vshCommandOptUInt to forbid negative number,
   and check if count is zero. (thanks Peter and Andrea)

  tools/virsh-domain.c | 9 +++--
  1 file changed, 7 insertions(+), 2 deletions(-)

Pushed after improving the commit message.


Thanks a lot for your quick review and help !


Cheers.



Luyao

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

Re: [libvirt] [PATCHv2] virsh: fix no error when pass a count <= 0 to setvcpus

[Andrea Bolognani]

On Thu, 2015-10-22 at 11:27 +0800, Luyao Huang wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1248277
> 
> When count <= 0, the client exit without set an error.
> 
> Signed-off-by: Luyao Huang 
> ---
> v2:
> - use vshCommandOptUInt to forbid negative number,
>   and check if count is zero. (thanks Peter and Andrea)
> 
>  tools/virsh-domain.c | 9 +++--
>  1 file changed, 7 insertions(+), 2 deletions(-)

Pushed after improving the commit message.

Cheers.

-- 
Andrea Bolognani
Software Engineer - Virtualization Team

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

Re: [libvirt] [PATCH 1/4] qemu: hostdev: Unify naming for qemuPrepareHost*Devices()

[Andrea Bolognani]

On Wed, 2015-10-21 at 20:40 -0400, Laine Stump wrote:
> On 10/21/2015 06:44 PM, John Ferlan wrote:
> > Typically when searching I can change the "virDomainFoo" type
> > string to
> > a "qemuDomainFoo" string and find the qemu API.  Here those you'll
> > see
> > there's a virHostDevPreparePCIDevices(), so in a way I'd expect a
> > qemuHostdevPreparePCIDevices
> > 
> > Since is a "qemu_hostdev.{c,h}" type change, then theoretically all
> > the
> > API's in here should be qemuHostdevFoo, right?
> > 
> > 
> > Rather than qemuPrepareFoo, qemuUpdateFoo, and
> > qemuDomainReAttachFoo
> > It'd be qemuHostdevPrepare, qemuHostdevUpdate, and
> > qemuHostdevReAttach
> > 
> > and your new API would be qemuHostdevUpdateActiveDevices
> 
> I had a similar reply queued up, but got interrupted by dinner:
> 
> You've changes all Hostdev into Host, but in all of these cases they
> really are  devices, so I think the function name is clearer
> if
> it has Hostdev in it rather than Host.
> 
> Also, if we're going to normalize the names, it might be better to
> try
> and fit in with the naming convention that we've tried to start using
> in
> other places - virModuleNameModifierAction or
> virModuleNameActionModifier (or in the case of functions in the qemu
> directory qemuModuleNameModifierAction() or
> qemuModuleNameActionModifier()), for example
> virNetdevMacVLanCreate().
> So instead of qemuPrepareHostdevPCIDevices() it could maybe be
> qemuHostdevPCIPrepareDevices(), qemuHostdevUSBPrepareDevices() and
> qemuHostdevSCSIPrepareDevices() (or maybe
> qemuHostdev*DevicesPrepare(),
> depending on how anal you want to be about putting the verb at the
> end)

I went with the smallest change possible to achieve some
sort of internal consistency, but I agree that being
consistent with the rest of the code is even better.

John's use case is very compelling IMHO, plus personally
something like

  qemuHostdevPreparePCIDevices()

feels way more natural than

  qemuHostdevPCIPrepareDevices()

so I'd go with his suggestion if that's okay with you.

Cheers.

-- 
Andrea Bolognani
Software Engineer - Virtualization Team

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

[libvirt] [PATCH] vz: add func to set shared drivers after libvirtd init

[Mikhail Feoktistov]

Built-in drivers in libvirt are initialized before libvirtd initialization.
Libvirt loads shared drivers on libvirtd initialization step.
For built-in drivers we can't set shared drivers, because they are not 
initialized yet.
This patch adds function to set shared drivers after libvirtd init.
---
 daemon/libvirtd.c|  4 
 src/libvirt.c| 41 +
 src/libvirt_internal.h   |  1 +
 src/libvirt_private.syms |  1 +
 4 files changed, 47 insertions(+)

diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index 250094b..aac1826 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -431,6 +431,10 @@ static void daemonInitialize(void)
 bhyveRegister();
 # endif
 #endif
+# ifdef WITH_VZ
+virAssignSharedDrivers("vz");
+virAssignSharedDrivers("Parallels");
+# endif
 }
 
 
diff --git a/src/libvirt.c b/src/libvirt.c
index 2602dde..4c4b7bd 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -1433,3 +1433,44 @@ virTypedParameterValidateSet(virConnectPtr conn,
 }
 return 0;
 }
+
+/**
+ * virAssignSharedDrivers:
+ * @name: name of connection driver
+ *
+ * This function fills in any empty pointers for shared drivers
+ * in connect driver structure
+ *
+ * Returns 0 in case of success, -1 in case of error
+*/
+int
+virAssignSharedDrivers(const char *name)
+{
+size_t i;
+
+if (name == NULL) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("Driver name must be specified"));
+return -1;
+}
+
+for (i = 0; i < virConnectDriverTabCount; i++) {
+if (STREQ(virConnectDriverTab[i]->hypervisorDriver->name, name)) {
+if (virConnectDriverTab[i]->interfaceDriver == NULL)
+virConnectDriverTab[i]->interfaceDriver = 
virSharedInterfaceDriver;
+if (virConnectDriverTab[i]->networkDriver == NULL)
+virConnectDriverTab[i]->networkDriver = virSharedNetworkDriver;
+if (virConnectDriverTab[i]->nodeDeviceDriver == NULL)
+virConnectDriverTab[i]->nodeDeviceDriver = 
virSharedNodeDeviceDriver;
+if (virConnectDriverTab[i]->nwfilterDriver == NULL)
+virConnectDriverTab[i]->nwfilterDriver = 
virSharedNWFilterDriver;
+if (virConnectDriverTab[i]->secretDriver == NULL)
+virConnectDriverTab[i]->secretDriver = virSharedSecretDriver;
+if (virConnectDriverTab[i]->storageDriver == NULL)
+virConnectDriverTab[i]->storageDriver = virSharedStorageDriver;
+break;
+   }
+}
+
+return 0;
+}
diff --git a/src/libvirt_internal.h b/src/libvirt_internal.h
index 1313b58..2a7227b 100644
--- a/src/libvirt_internal.h
+++ b/src/libvirt_internal.h
@@ -289,4 +289,5 @@ virTypedParameterValidateSet(virConnectPtr conn,
  virTypedParameterPtr params,
  int nparams);
 
+int virAssignSharedDrivers(const char *name);
 #endif
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index be6ee19..340555a 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -943,6 +943,7 @@ virFDStreamSetInternalCloseCb;
 
 
 # libvirt_internal.h
+virAssignSharedDrivers;
 virConnectSupportsFeature;
 virDomainMigrateBegin3;
 virDomainMigrateBegin3Params;
-- 
1.8.3.1

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

Re: [libvirt] [PATCH v2 0/2] tests: Add nodeinfo test data utility scripts

[Martin Kletzander]

On Thu, Oct 22, 2015 at 10:42:54AM +0200, Andrea Bolognani wrote:

On Wed, 2015-10-21 at 17:43 -0400, John Ferlan wrote:

> Andrea Bolognani (2):
>   tests: Add script to display nodeinfo test data
>   tests: Add script to copy nodeinfo test data from host
>
>  tests/nodeinfodata/copy-from-host.sh | 113
> +++
>  tests/nodeinfodata/display.sh| 113
> +++
>  2 files changed, 226 insertions(+)
>  create mode 100755 tests/nodeinfodata/copy-from-host.sh
>  create mode 100755 tests/nodeinfodata/display.sh

I'm ambivalent on this pair.

Not sure what the value of patch 1 is?  What should I expect to see
given the arguments?  What does "ppc64_cpu --info" show?  Perhaps the
better question is - if you run on each directory in nodeinfodata do
you
get what you expect?


I've run the script on every existing dataset and the output
was correct, as far as I can tell.

The script was immensely useful to me back when I was
implementing changes to the way the nodeinfo code counts
CPUs when subcorese are involved, eg.

 $ ./display.sh linux-subcores3 8
 Threads per core: 8
 Present CPUs: 0-159

 Core   0:0 1 2 3 4 5 6 7
 Core   1:8*9101112131415
 Core   2:   1617181920212223
 Core   3:   2425262728293031
 Core   4:   3233343536373839
 Core   5:   40*   41424344454647
 Core   6:   48*   49505152535455
 Core   7:   56*   57585960616263
 Core   8:   6465666768*   697071
 Core   9:   72*   73747576777879
 Core  10:   80*   81828384858687
 Core  11:   88*   89909192939495
 Core  12:   96*   979899   100   101   102   103
 Core  13:  104*  105   106   107   108   109   110   111
 Core  14:  112*  113   114   115   116   117   118   119
 Core  15:  120   121   122   123   124   125   126   127
 Core  16:  128*  129   130   131   132   133   134   135
 Core  17:  136*  137   138   139   140   141   142   143
 Core  18:  144   145   146   147   148   149   150   151
 Core  19:  152*  153*  154*  155*  156*  157*  158*  159*

You can see at a glance there's something wrong with this
configuration - why is CPU 68 online? What about the last
line? This kind of report is especially useful when dealing
with processors with a high number of CPUs.


As for patch 2, one would have to know they should use the
copy-from-host.sh script. Perhaps what might be better and/or
somewhat
more interesting on this one is some make check rule that scans the
nodeinfodata trees looking for files that shouldn't be there. That
way
if someone does use their own methodology to copy over the tree we'd
know it (and could message to use the copy-from-host.sh script...


I agree, as it stands it's not very discoverable, plus
adding the check you suggest would also prevent something
like e739d95 from ever being needed again.

I'll work on that as soon as I have some time.



Maybe simple .gitignore entry would suffice.


Cheers.

--
Andrea Bolognani
Software Engineer - Virtualization Team


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

[libvirt] [RFC] Simplifying usage of {Enter, Exit}Monitor and {Begin/End}Job

[Jiri Denemark]

Hi all.

Looking at the qemu driver code, we have a lot of code with the
following pattern:

if (doSomething() < 0)
goto cleanup;

if (qemuDomainObjBeginJob() < 0)
goto cleanup;

if (doSomethingElse() < 0)
goto endjob;

qemuDomainObjEnterMonitor();

if (qemuMonitorSomething() < 0) {
qemuDomainObjExitMonitor();
goto endjob;
}

if (qemuMonitorSomethingElse() < 0) {
qemuDomainObjExitMonitor();
goto endjob;
}

if (qemuDomainObjExitMonitor() < 0)
goto endjob;

...

ret = 0;

 endjob:
qemuDomainObjEndJob();

 cleanup:
...
return ret;

Sometimes qemuDomainObjExitMonitor is in its own label instead of being
explicitly called on every single error path. Sometimes
qemuDomainObjEndJob is called explicitly followed by goto cleanup. In
either case, it's pretty easy to get this wrong. It's too easy to jump
to a wrong label (especially when moving code around) or forget to call
the appropriate exit function before jumping to a label.

So what if we make the code more robust by changing who needs to track
whether we are in a monitor or have a job. Now it's the caller that
tracks it while I think we could teach the job/monitor APIs themselves
to track the state:

bool inJob = false;
bool inMonitor = false;

if (doSomething() < 0)
goto cleanup;

if (qemuDomainObjBeginJob(..., ) < 0)
goto cleanup;

if (doSomethingElse() < 0)
goto cleanup;

qemuDomainObjEnterMonitor(..., );

if (qemuMonitorSomething() < 0)
goto cleanup;

if (qemuMonitorSomethingElse() < 0)
goto cleanup;

if (qemuDomainObjExitMonitor(..., ) < 0)
goto cleanup;

...

ret = 0;

 cleanup:
if (qemuDomainObjExitMonitor(..., ) < 0)
ret = -1;
qemuDomainObjEndJob(..., );
...
return ret;


It's not a lot shorter or longer but it saves us from jumping to
different labels and it makes sure we always exit the monitor and end
the job.

So is it worth the effort or do you thing it's even worse than before or
do you have any other ideas?

Thanks,

Jirka

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

Re: [libvirt] strange stale qemu processes after domain shutdown

[Vasiliy Tolstov]

2015-10-22 17:38 GMT+03:00 Michal Privoznik :
> These are probably left over after daemon restart. I mean, when deamon
> restarts, it reloads the state XML. However, if there's something wrong
> (e.g. unknown device), the state XML is ignored and libvirt thinks the
> domain is shut off, leaving qemu process behind. The unknown device can
> happen if you run older daemon than the domain has been started with.
>

Daemon does not restarts, also to note - domain started without
define. Libvirt version does not changed

> You can check daemon debug logs and see how it loads domain state XMLs.
> If there's an error somewhere it's possibly this case.
>

If domain is not defined, if i'm enable debug in libvirt and restart
it, this is not helps because qemu process already stalled and libvirt
does not know about this domain because they not defined.

> Of course, the other option would be that we have a bug somewhere where
> we leave qemu process behind. But the former is more likely than the latter.


I'm try to add to monitoring (difference with virsh --state-running
--name vs ps auxww | grep qemu-system | grep -v grep | wc -l) and
investigate this issue...

-- 
Vasiliy Tolstov,
e-mail: v.tols...@selfip.ru

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

Re: [libvirt] strange stale qemu processes after domain shutdown

[Michal Privoznik]

On 15.10.2015 20:41, Vasiliy Tolstov wrote:
> I have 58 active domains with status running, and 62
> qemu-system-x86_64 processes.
> After investigating this issue, i found problem domains.
> How to fix this issue and not lost this qemu processes?

These are probably left over after daemon restart. I mean, when deamon
restarts, it reloads the state XML. However, if there's something wrong
(e.g. unknown device), the state XML is ignored and libvirt thinks the
domain is shut off, leaving qemu process behind. The unknown device can
happen if you run older daemon than the domain has been started with.

You can check daemon debug logs and see how it loads domain state XMLs.
If there's an error somewhere it's possibly this case.

Of course, the other option would be that we have a bug somewhere where
we leave qemu process behind. But the former is more likely than the latter.

Michal

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

Re: [libvirt] [RFC] Simplifying usage of {Enter, Exit}Monitor and {Begin/End}Job

[Daniel P. Berrange]

On Thu, Oct 22, 2015 at 01:47:28PM +0200, Jiri Denemark wrote:
> Hi all.
> 
> Looking at the qemu driver code, we have a lot of code with the
> following pattern:
> 
> if (doSomething() < 0)
> goto cleanup;
> 
> if (qemuDomainObjBeginJob() < 0)
> goto cleanup;
> 
> if (doSomethingElse() < 0)
> goto endjob;
> 
> qemuDomainObjEnterMonitor();
> 
> if (qemuMonitorSomething() < 0) {
> qemuDomainObjExitMonitor();
> goto endjob;
> }
> 
> if (qemuMonitorSomethingElse() < 0) {
> qemuDomainObjExitMonitor();
> goto endjob;
> }
> 
> if (qemuDomainObjExitMonitor() < 0)
> goto endjob;
> 
> ...
> 
> ret = 0;
> 
>  endjob:
> qemuDomainObjEndJob();
> 
>  cleanup:
> ...
> return ret;
> 
> Sometimes qemuDomainObjExitMonitor is in its own label instead of being
> explicitly called on every single error path. Sometimes
> qemuDomainObjEndJob is called explicitly followed by goto cleanup. In
> either case, it's pretty easy to get this wrong. It's too easy to jump
> to a wrong label (especially when moving code around) or forget to call
> the appropriate exit function before jumping to a label.
> 
> So what if we make the code more robust by changing who needs to track
> whether we are in a monitor or have a job. Now it's the caller that
> tracks it while I think we could teach the job/monitor APIs themselves
> to track the state:
> 
> bool inJob = false;
> bool inMonitor = false;
> 
> if (doSomething() < 0)
> goto cleanup;
> 
> if (qemuDomainObjBeginJob(..., ) < 0)
> goto cleanup;
> 
> if (doSomethingElse() < 0)
> goto cleanup;
> 
> qemuDomainObjEnterMonitor(..., );
> 
> if (qemuMonitorSomething() < 0)
> goto cleanup;
> 
> if (qemuMonitorSomethingElse() < 0)
> goto cleanup;
> 
> if (qemuDomainObjExitMonitor(..., ) < 0)
> goto cleanup;
> 
> ...
> 
> ret = 0;
> 
>  cleanup:
> if (qemuDomainObjExitMonitor(..., ) < 0)
> ret = -1;
> qemuDomainObjEndJob(..., );
> ...
> return ret;
> 
> 
> It's not a lot shorter or longer but it saves us from jumping to
> different labels and it makes sure we always exit the monitor and end
> the job.
> 
> So is it worth the effort or do you thing it's even worse than before or
> do you have any other ideas?

On a related topic, we don't have great error reporting in the (usually
unlikely) scenario that we get a stuck job / timeout. I've long thought
it could be desirable to record some metadata when we start jobs, such
as the __FUNC__ of the method which started the job, so when we report
an error we can include that info as a diagnostic aid. This would
have to be against the qemuDomainObjPrivPtr struct. THis makes me
think that using the separate bool inJob/inMonitor stack variables
is not required.

We could just add

   int threadid;
   bool inJob;
   bool inMonitor;
   const char *jobfunc;

to qemuDomainObjPrivPtr. That way you don't need to modify the
Enter/Exit functions to add extra arguments - we just track
everything internally. When exiting, we'd compare against the
threadid, to make sure we don't accidentally relaase a different
thread's job.


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

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

Re: [libvirt] [RFC] Simplifying usage of {Enter, Exit}Monitor and {Begin/End}Job

[Martin Kletzander]

On Thu, Oct 22, 2015 at 01:47:28PM +0200, Jiri Denemark wrote:

Hi all.

Looking at the qemu driver code, we have a lot of code with the
following pattern:

   if (doSomething() < 0)
   goto cleanup;

   if (qemuDomainObjBeginJob() < 0)
   goto cleanup;

   if (doSomethingElse() < 0)
   goto endjob;

   qemuDomainObjEnterMonitor();

   if (qemuMonitorSomething() < 0) {
   qemuDomainObjExitMonitor();
   goto endjob;
   }

   if (qemuMonitorSomethingElse() < 0) {
   qemuDomainObjExitMonitor();
   goto endjob;
   }

   if (qemuDomainObjExitMonitor() < 0)
   goto endjob;

   ...

   ret = 0;

endjob:
   qemuDomainObjEndJob();

cleanup:
   ...
   return ret;

Sometimes qemuDomainObjExitMonitor is in its own label instead of being
explicitly called on every single error path. Sometimes
qemuDomainObjEndJob is called explicitly followed by goto cleanup. In
either case, it's pretty easy to get this wrong. It's too easy to jump
to a wrong label (especially when moving code around) or forget to call
the appropriate exit function before jumping to a label.

So what if we make the code more robust by changing who needs to track
whether we are in a monitor or have a job. Now it's the caller that
tracks it while I think we could teach the job/monitor APIs themselves
to track the state:

   bool inJob = false;
   bool inMonitor = false;

   if (doSomething() < 0)
   goto cleanup;

   if (qemuDomainObjBeginJob(..., ) < 0)
   goto cleanup;

   if (doSomethingElse() < 0)
   goto cleanup;

   qemuDomainObjEnterMonitor(..., );

   if (qemuMonitorSomething() < 0)
   goto cleanup;

   if (qemuMonitorSomethingElse() < 0)
   goto cleanup;

   if (qemuDomainObjExitMonitor(..., ) < 0)
   goto cleanup;

   ...

   ret = 0;

cleanup:
   if (qemuDomainObjExitMonitor(..., ) < 0)
   ret = -1;
   qemuDomainObjEndJob(..., );
   ...
   return ret;


It's not a lot shorter or longer but it saves us from jumping to
different labels and it makes sure we always exit the monitor and end
the job.

So is it worth the effort or do you thing it's even worse than before or
do you have any other ideas?



It's good and similar to what I have though of few times as well.  I
have just two ideas for making it even better.  Firstly, there is one
thing we need to fix and that is that bunch of APIs should have a job
and they don't.  Basically every API shold have a job, even if it's
just a QEMU_JOB_QUERY, that's why we have masks that say which jobs
are allowed when.  So what if we had one function that fetches the
domain object and sets a job there?  That would solve few mishaps and
made the code a bit shorter.  Another thing is that since you know
whether to call ExitMonitor and EndJob, you can incorporate them into
one function, like there was the qemuDomObjEndAPI() if you put the
inMonitor and inJob variables somewhere accessible.  Later on, if we
introduce job control everywhere, we could make it part of
virDomainObjEndAPI() and the starting function would be also
driver-agnostic.  But that's for the future (and we all know the
future was yesterday).  Anyway, I think it's a good idea and with some
polish (lowercase 'p') it'll rock.


Thanks,

Jirka

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


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

Re: [libvirt] [RFC] Simplifying usage of {Enter, Exit}Monitor and {Begin/End}Job

[Daniel P. Berrange]

On Thu, Oct 22, 2015 at 03:23:49PM +0200, Jiri Denemark wrote:
> On Thu, Oct 22, 2015 at 13:52:29 +0100, Daniel P. Berrange wrote:
> ...
> > On a related topic, we don't have great error reporting in the (usually
> > unlikely) scenario that we get a stuck job / timeout. I've long thought
> > it could be desirable to record some metadata when we start jobs, such
> > as the __FUNC__ of the method which started the job, so when we report
> > an error we can include that info as a diagnostic aid.
> 
> Do you mean something like
> 
> virsh # resume cd
> error: Failed to resume domain cd
> error: Timed out during operation: cannot acquire state change lock
> (held by remoteDispatchDomainSuspend)
> 
> This was implemented by v1.2.13-295-gb79f25e
> 
> > This would
> > have to be against the qemuDomainObjPrivPtr struct. THis makes me
> > think that using the separate bool inJob/inMonitor stack variables
> > is not required.
> > 
> > We could just add
> > 
> >int threadid;
> >bool inJob;
> >bool inMonitor;
> >const char *jobfunc;
> > 
> > to qemuDomainObjPrivPtr. That way you don't need to modify the
> > Enter/Exit functions to add extra arguments - we just track
> > everything internally. When exiting, we'd compare against the
> > threadid, to make sure we don't accidentally relaase a different
> > thread's job.
> 
> Yeah, as long as we can make sure threadid is unique and stable:
> 
> /* These next two functions are for debugging only, since they are not
>  * guaranteed to give unique values for distinct threads on all
>  * architectures, nor are the two functions guaranteed to give the same
>  * value for the same thread. */
> unsigned long long virThreadSelfID(void);
> unsigned long long virThreadID(virThreadPtr thread);
> 
> so far we avoided using thread IDs for anything critical.

So technically that comment is correct, since we're casting the
pthread_t type to an unsigned long long, also you cannot directly
compare pthread_t == pthread_t.

POSIX does however provide  pthread_equal() to allow you to compare
whether 2 pthread_t values point to the same thread.

  http://man7.org/linux/man-pages/man3/pthread_self.3.html
  http://man7.org/linux/man-pages/man3/pthread_equal.3.html

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

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

Re: [libvirt] [RFC] Simplifying usage of {Enter, Exit}Monitor and {Begin/End}Job

[Jiri Denemark]

On Thu, Oct 22, 2015 at 13:52:29 +0100, Daniel P. Berrange wrote:
...
> On a related topic, we don't have great error reporting in the (usually
> unlikely) scenario that we get a stuck job / timeout. I've long thought
> it could be desirable to record some metadata when we start jobs, such
> as the __FUNC__ of the method which started the job, so when we report
> an error we can include that info as a diagnostic aid.

Do you mean something like

virsh # resume cd
error: Failed to resume domain cd
error: Timed out during operation: cannot acquire state change lock
(held by remoteDispatchDomainSuspend)

This was implemented by v1.2.13-295-gb79f25e

> This would
> have to be against the qemuDomainObjPrivPtr struct. THis makes me
> think that using the separate bool inJob/inMonitor stack variables
> is not required.
> 
> We could just add
> 
>int threadid;
>bool inJob;
>bool inMonitor;
>const char *jobfunc;
> 
> to qemuDomainObjPrivPtr. That way you don't need to modify the
> Enter/Exit functions to add extra arguments - we just track
> everything internally. When exiting, we'd compare against the
> threadid, to make sure we don't accidentally relaase a different
> thread's job.

Yeah, as long as we can make sure threadid is unique and stable:

/* These next two functions are for debugging only, since they are not
 * guaranteed to give unique values for distinct threads on all
 * architectures, nor are the two functions guaranteed to give the same
 * value for the same thread. */
unsigned long long virThreadSelfID(void);
unsigned long long virThreadID(virThreadPtr thread);

so far we avoided using thread IDs for anything critical.

Jirka

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

Re: [libvirt] [PATCH v2 0/2] tests: Add nodeinfo test data utility scripts

[Andrea Bolognani]

On Thu, 2015-10-22 at 13:27 +0200, Martin Kletzander wrote:
> > > As for patch 2, one would have to know they should use the
> > > copy-from-host.sh script. Perhaps what might be better and/or
> > > somewhat
> > > more interesting on this one is some make check rule that scans
> > > the
> > > nodeinfodata trees looking for files that shouldn't be there.
> > > That
> > > way
> > > if someone does use their own methodology to copy over the tree
> > > we'd
> > > know it (and could message to use the copy-from-host.sh script...
> > 
> > I agree, as it stands it's not very discoverable, plus
> > adding the check you suggest would also prevent something
> > like e739d95 from ever being needed again.
> > 
> > I'll work on that as soon as I have some time.
> > 
> 
> Maybe simple .gitignore entry would suffice.

Problem is, .gitignore works well if you have the list of
files you want to avoid, but we have the complementary
information: the list of files we want to keep.

Cheers.

-- 
Andrea Bolognani
Software Engineer - Virtualization Team

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

Re: [libvirt] [PATCH 1/4] virsh-domain: use correct base for virStrToLong_ui

[Laine Stump]

On 10/21/2015 08:22 AM, Pavel Hrdina wrote:

While parsing device addresses we should use correct base and don't
count on auto-detect.  For example, PCI address uses hex numbers, but
each number starting with 0 will be auto-detected as octal number and
that's wrong.  Another wrong use-case is for PCI address if for example
bus is 10, than it's incorrectly parsed as decimal number.

PCI and CCW addresses have all values as hex numbers, IDE and SCSI
addresses are in decimal numbers.


I've seen examples for PCI that use decimal a number for the slot (which 
is the one item that's likely to have a value > 7 unless you have a 
system with a whole bunch of PCI controllers)[*], and those that use hex 
numbers always prefix the number with "0x". libvirt itself always 
prefixes the domain, bus, and slot with 0x, so an auto-detected base 
will always get those right.


So I I think the existing code is correct, and don't see an upside to 
making this restriction/invisible change in semantics, and it could 
potentially lead to incorrect results if someone is thinking that 
they're entering decimal numbers.


[*] There was one particular document that even went to the trouble of 
explaining how to convert the hex value of slot into a decimal number to 
use in the libvirt config!




Signed-off-by: Pavel Hrdina 
---
  tools/virsh-domain.c | 30 +++---
  1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 4191548..e8503ec 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -455,19 +455,19 @@ static int str2PCIAddress(const char *str, struct 
PCIAddress *pciAddr)
  
  domain = (char *)str;
  
-if (virStrToLong_ui(domain, , 0, >domain) != 0)

+if (virStrToLong_ui(domain, , 16, >domain) != 0)
  return -1;
  
  bus++;

-if (virStrToLong_ui(bus, , 0, >bus) != 0)
+if (virStrToLong_ui(bus, , 16, >bus) != 0)
  return -1;
  
  slot++;

-if (virStrToLong_ui(slot, , 0, >slot) != 0)
+if (virStrToLong_ui(slot, , 16, >slot) != 0)
  return -1;
  
  function++;

-if (virStrToLong_ui(function, NULL, 0, >function) != 0)
+if (virStrToLong_ui(function, NULL, 16, >function) != 0)
  return -1;
  
  return 0;

@@ -484,15 +484,15 @@ static int str2SCSIAddress(const char *str, struct 
SCSIAddress *scsiAddr)
  
  controller = (char *)str;
  
-if (virStrToLong_uip(controller, , 0, >controller) != 0)

+if (virStrToLong_uip(controller, , 10, >controller) != 0)
  return -1;
  
  bus++;

-if (virStrToLong_uip(bus, , 0, >bus) != 0)
+if (virStrToLong_uip(bus, , 10, >bus) != 0)
  return -1;
  
  unit++;

-if (virStrToLong_ullp(unit, NULL, 0, >unit) != 0)
+if (virStrToLong_ullp(unit, NULL, 10, >unit) != 0)
  return -1;
  
  return 0;

@@ -509,15 +509,15 @@ static int str2IDEAddress(const char *str, struct 
IDEAddress *ideAddr)
  
  controller = (char *)str;
  
-if (virStrToLong_ui(controller, , 0, >controller) != 0)

+if (virStrToLong_ui(controller, , 10, >controller) != 0)
  return -1;
  
  bus++;

-if (virStrToLong_ui(bus, , 0, >bus) != 0)
+if (virStrToLong_ui(bus, , 10, >bus) != 0)
  return -1;
  
  unit++;

-if (virStrToLong_ui(unit, NULL, 0, >unit) != 0)
+if (virStrToLong_ui(unit, NULL, 10, >unit) != 0)
  return -1;
  
  return 0;

@@ -534,15 +534,15 @@ static int str2CCWAddress(const char *str, struct 
CCWAddress *ccwAddr)
  
  cssid = (char *)str;
  
-if (virStrToLong_ui(cssid, , 0, >cssid) != 0)

+if (virStrToLong_ui(cssid, , 16, >cssid) != 0)
  return -1;
  
  ssid++;

-if (virStrToLong_ui(ssid, , 0, >ssid) != 0)
+if (virStrToLong_ui(ssid, , 16, >ssid) != 0)
  return -1;
  
  devno++;

-if (virStrToLong_ui(devno, NULL, 0, >devno) != 0)
+if (virStrToLong_ui(devno, NULL, 16, >devno) != 0)
  return -1;
  
  return 0;

@@ -739,8 +739,8 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
  } else if (STRPREFIX((const char *)target, "hd")) {
  if (diskAddr.type == DISK_ADDR_TYPE_IDE) {
  virBufferAsprintf(,
-  "\n",
+  "\n",
diskAddr.addr.ide.controller, 
diskAddr.addr.ide.bus,
diskAddr.addr.ide.unit);
  } else {


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

[libvirt] outreachy 2015

[mesode akwe]

hello!

i am mesode from cameroon and am interested in participating in the
libvirt-designer enhancement project.
where do i go from here?
thanks.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list