date:20131114

Re: [libvirt] [PATCH 2/2] Error out on unterminated arrays and objects in JSON parser

2013-11-14 Thread Martin Kletzander

On Tue, Nov 05, 2013 at 03:59:56PM +0100, Ján Tomko wrote:
> ---
>  src/util/virjson.c | 9 -
>  tests/jsontest.c   | 5 +
>  2 files changed, 13 insertions(+), 1 deletion(-)
> 

ACK,

Martin


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

Re: [libvirt] [PATCH 1/2] Test if JSON parser fails on invalid input

2013-11-14 Thread Martin Kletzander

On Tue, Nov 05, 2013 at 03:59:55PM +0100, Ján Tomko wrote:
> ---
>  tests/jsontest.c | 13 +
>  1 file changed, 13 insertions(+)
> 
> diff --git a/tests/jsontest.c b/tests/jsontest.c
> index 6add816..808a2ea 100644
> --- a/tests/jsontest.c
> +++ b/tests/jsontest.c
> @@ -145,6 +145,10 @@ mymain(void)
>  #define DO_TEST_PARSE(name, doc)\
>  DO_TEST_FULL(name, FromString, doc, NULL, true)
>  
> +#define DO_TEST_PARSE_FAIL(name, doc)   \
> +DO_TEST_FULL(name, FromString, doc, NULL, false)
> +
> +

You're testing virJSONValueFromString() here ...

>  DO_TEST_PARSE("Simple", "{\"return\": {}, \"id\": \"libvirt-1\"}");
>  DO_TEST_PARSE("NotSoSimple", "{\"QMP\": {\"version\": {\"qemu\":"
>  "{\"micro\": 91, \"minor\": 13, \"major\": 0},"
> @@ -188,6 +192,15 @@ mymain(void)
>  DO_TEST_FULL("add and remove", AddRemove,
>   "[ 1 ]", NULL, false);
>  
> +
> +DO_TEST_PARSE_FAIL("nothing", "");
> +DO_TEST_PARSE_FAIL("number with garbage", "2345b45");
> +DO_TEST_PARSE_FAIL("float with garbage", "0.0314159ee+100");
> +DO_TEST_PARSE_FAIL("unterminated string", "\"The meaning of lif");

... that not only cannot parse these, but it can't even parse proper
numbers (e.g. "12345", "1.2e+34"), because it expects object (array or
map).  You can do two things with this.  Either create new
testJSON...() which will test numbers only (requires v2) or squash
this in (which makes it work properly and shows that also):

diff --git a/tests/jsontest.c b/tests/jsontest.c
index e806b6f..a7ee2ef 100644
--- a/tests/jsontest.c
+++ b/tests/jsontest.c
@@ -193,10 +193,18 @@ mymain(void)
  "[ 1 ]", NULL, false);


+DO_TEST_PARSE("almost nothing", "[]");
 DO_TEST_PARSE_FAIL("nothing", "");
-DO_TEST_PARSE_FAIL("number with garbage", "2345b45");
-DO_TEST_PARSE_FAIL("float with garbage", "0.0314159ee+100");
-DO_TEST_PARSE_FAIL("unterminated string", "\"The meaning of lif");
+
+DO_TEST_PARSE("number without garbage", "[ 234545 ]");
+DO_TEST_PARSE_FAIL("number with garbage", "[ 2345b45 ]");
+
+DO_TEST_PARSE("float without garbage", "[ 0.0314159e+100 ]");
+DO_TEST_PARSE_FAIL("float with garbage", "[ 0.0314159ee+100 ]");
+
+DO_TEST_PARSE("string", "[ \"The meaning of life\" ]");
+DO_TEST_PARSE_FAIL("unterminated string", "[ \"The meaning of lif ]");
+
 DO_TEST_PARSE_FAIL("unterminated array", "[ 1, 2, 3");
 DO_TEST_PARSE_FAIL("object with numeric keys", "{ 1:1, 2:1, 3:2 }");
 DO_TEST_PARSE_FAIL("unterminated object", "{ \"1\":1, \"2\":1, \"3\":2");
--

ACK if you want to use the second option.

Martin


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

[libvirt] [PATCH] Fix migration with qemu 1.6

2013-11-14 Thread whitearchey


QEMU 1.6.0 introduced new migration status: setup
Libvirt does not expect such string in QMP and refuses to migrate with error
"unexpected migration status in setup"

This patch fixes it.

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index a3d986f..4b5fdba 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -1644,6 +1644,10 @@ qemuMigrationUpdateJobStatus(virQEMUDriverPtr driver,
_("%s: %s"), job, _("is not active"));
 break;

+case QEMU_MONITOR_MIGRATION_STATUS_SETUP:
+ret = 0;
+break;
+
 case QEMU_MONITOR_MIGRATION_STATUS_ACTIVE:
 priv->job.info.fileTotal = priv->job.status.disk_total;
 priv->job.info.fileRemaining = priv->job.status.disk_remaining;
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 30315b3..0d4598e 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -114,7 +114,7 @@ VIR_ONCE_GLOBAL_INIT(qemuMonitor)

 VIR_ENUM_IMPL(qemuMonitorMigrationStatus,
   QEMU_MONITOR_MIGRATION_STATUS_LAST,
-  "inactive", "active", "completed", "failed", "cancelled")
+  "inactive", "active", "completed", "failed", "cancelled", 
"setup")

 VIR_ENUM_IMPL(qemuMonitorMigrationCaps,
   QEMU_MONITOR_MIGRATION_CAPS_LAST,
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index f893b1f..eabf000 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -397,6 +397,7 @@ enum {
 QEMU_MONITOR_MIGRATION_STATUS_COMPLETED,
 QEMU_MONITOR_MIGRATION_STATUS_ERROR,
 QEMU_MONITOR_MIGRATION_STATUS_CANCELLED,
+QEMU_MONITOR_MIGRATION_STATUS_SETUP,

 QEMU_MONITOR_MIGRATION_STATUS_LAST
 };

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

Re: [libvirt] [PATCH v2] virsh domxml-from-native to treat SCSI as the bus type for pseries by default

2013-11-14 Thread Li Zhang


On 2013年11月14日 18:02, Ján Tomko wrote:

On 10/28/2013 10:20 AM, Shivaprasad G Bhat wrote:

The bus type IDE being enum Zero, the bus type on pseries system appears as IDE 
for all the disk types. Pseries platform needs this to appear as SCSI instead 
of IDE.

Signed-off-by: Shivaprasad G Bhat 
---
  src/qemu/qemu_domain.c |   11 +++
  1 file changed, 11 insertions(+)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index b8aec2d..df06c13 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -827,6 +827,12 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
  if (dev->type == VIR_DOMAIN_DEVICE_DISK) {
  virDomainDiskDefPtr disk = dev->data.disk;
  
+if ((def->os.arch == VIR_ARCH_PPC64) &&

+def->os.machine && STREQ(def->os.machine, "pseries") &&
+(disk->bus == VIR_DOMAIN_DISK_BUS_IDE)) {
+disk->bus = VIR_DOMAIN_DISK_BUS_SCSI;
+}
+
  /* both of these require data from the driver config */
  if (driver && (cfg = virQEMUDriverGetConfig(driver))) {
  /* assign default storage format and driver according to config */
@@ -868,6 +874,11 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
  (def->os.arch == VIR_ARCH_S390 || def->os.arch == VIR_ARCH_S390X))
  dev->data.chr->targetType = VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO;
  
+if (dev->type == VIR_DOMAIN_DEVICE_CONTROLLER &&

+dev->data.controller->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE &&
+def->os.machine && STREQ(def->os.machine, "pseries"))
+dev->data.controller->type = VIR_DOMAIN_CONTROLLER_TYPE_SCSI;
+
  /* set the default USB model to none for s390 unless an address is found 
*/
  if (dev->type == VIR_DOMAIN_DEVICE_CONTROLLER &&
  dev->data.controller->type == VIR_DOMAIN_CONTROLLER_TYPE_USB &&


This would also affect XML parsing, as these PostParse functions are called
when parsing the XML too, not just when doing a XML->native translation.

This also affect all the disks specified on the command line. You shouldn't
assume the disk is SCSI when IDE was explicitly specified.


This patch is to parse "-hda" as SCSI device type because Power doesn't 
support IDE device.
I am not sure whether it's necessary to keep IDE device type for Power 
in libvirt.

Currently, QEMU set "-hda" as SCSI for Power platform.



Jan




--

Li Zhang
IBM China Linux Technology Centre

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

[libvirt] Fwd: Re: the case for volatile nwfilters

2013-11-14 Thread Stefan Berger



For some reason this message could not be sent to the list.

On 10/30/2013 05:57 AM, Laine Stump wrote:

On 10/29/2013 06:33 PM, Dan Kenigsberg wrote:

I'd like oVirt to make a more extensive usage of libvirt's nwfilters in
order to implement security groups, i.e. which protocol/port/host should
be open on an interface.

Since oVirt is cetrally-managed by ovirt-engine, filter definitions
should be edited there and kept in its database. However, libivrt's
domain xml requires to have a locally-defined filter as well:

   
 
   
 
   

We can leave with it by defining an ad-hoc filter before staring a VM,
deleting it after the VM stops, and collecting garbage (due to system
crashes) occasionally.

It would be nicer if we could instead have just-in-time filter
definition such as

   
 
   
  
  
  
   
 
   

avoiding nwfilter persistence. Would something like this be beneficial
to other libvirt users? Would it be easy to implement within libvirt?

My first thought when I saw the subject line was of adding a transient
"define and start" API similar to that used for domains and networks,
but of course nwfilters have no concept of "start" or "destroy", so they
wouldn't be able to follow the same semantics as transient domains and
networks anyway.

Most likely Stefan made nwfilters use in domains all be references to
the actual nwfilters rather than having them contained in order to 1)
save duplication in the configuration now, and 2) potentially save
duplication in the kernel iptables rules in the future. I can see the
convenience, but wonder how much inefficiency it could lead to.

Structurally it makes sense though, and the xml has conveniently named
the existing element as  so that  is still available
- was there some premonition of this being requested in the future? :-)

I recall that Stefan has been extremely busy elsewhere and unable to
completely follow libvir-list for awhile, so I'm Cc'ing him - Stefan, do
you have any opinion/wisdom on this?



Why can the client not call nwfilter-define to define a filter and have
the filter referenced from the domain XML?
Also many of the items in the domain XML only become active after a
reboot of the VM (afaik) and I didn't want to change that with nwfilter.
So you can at any time replace the referenced nwfilter and it will be
instantiated when/while the VM is running or rejected if it cannot be
instantiated.

   Stefan



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

Re: [libvirt] [PATCHv2] maint: Kill usage of atoi()

2013-11-14 Thread Eric Blake

On 11/14/2013 02:22 PM, Peter Krempa wrote:

>>> +
>>> +exclude_file_name_regexp--sc_prohibit_atoi= \
>>> +   ^examples/.*\.[ch]$$
>>
>> Drop this hunk.  None of our examples/* use atoi to begin with, so
>> there's no reason to exclude them from the syntax check.
>>
>> ACK with those fixes.
>>
> 
> This isn't true unfortunately:

Huh, wonder what grep I did that missed that fact?

> 
> ~/libvirt $ git grep atoi examples/
> examples/domsuspend/suspend.c:id = atoi(argv[1]);
> 
> and I wanted to avoid changing that file. But if you insist I can tune
> that one too.

Yes, it's worth fixing, because our examples should never teach people
to use bad coding practices.  atoi() is broken by design, and strtol(),
while more awkward to use, is just as portably present as atoi().

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



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

Re: [libvirt] [PATCHv2] maint: Kill usage of atoi()

2013-11-14 Thread Peter Krempa

On 11/14/13 17:53, Eric Blake wrote:
> On 11/14/2013 09:48 AM, Peter Krempa wrote:
>> Kill the use of atoi() and introduce syntax check to forbit it and it's
> 
> s/forbit/forbid/
> 
>> friends (atol, atoll, atof, atoq).
>>
>> Also fix a typo in variable name holding the cylinders count of a disk
>> pool (apparently unused).
>> ---
>>  cfg.mk |  9 +
>>  src/conf/storage_conf.h|  2 +-
>>  src/qemu/qemu_command.c|  8 ++--
>>  src/storage/storage_backend_disk.c | 14 +-
>>  src/xen/xend_internal.c| 17 +
>>  5 files changed, 38 insertions(+), 12 deletions(-)
>>
>> diff --git a/cfg.mk b/cfg.mk
> 
>> @@ -1040,3 +1046,6 @@ exclude_file_name_regexp--sc_prohibit_int_ijk = \
>>
>>  exclude_file_name_regexp--sc_prohibit_getenv = \
>>^tests/.*\.[ch]$$
>> +
>> +exclude_file_name_regexp--sc_prohibit_atoi= \
>> +^examples/.*\.[ch]$$
> 
> Drop this hunk.  None of our examples/* use atoi to begin with, so
> there's no reason to exclude them from the syntax check.
> 
> ACK with those fixes.
> 

This isn't true unfortunately:

~/libvirt $ git grep atoi examples/
examples/domsuspend/suspend.c:id = atoi(argv[1]);

and I wanted to avoid changing that file. But if you insist I can tune
that one too.

Peter




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

[libvirt] Finding Primary or Bootable disk

2013-11-14 Thread Umar Draz

Hi

I am working with php-libvirt,

How I can find the primary or bootable disk device of a virtual machine?

Br.

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

Re: [libvirt] [PATCH] Properly unref a connection with a close callback

2013-11-14 Thread Michal Privoznik

On 14.11.2013 18:24, Ján Tomko wrote:
> The connection pointer in the closeCallback data was never
> initialized, making the unref in remoteClientCloseFunc a no-op.
> 
> This fixes the following leak in virsh when the daemon closes
> the connection unexpectedly:
> 
> 1,179 (288 direct, 891 indirect) bytes in 1 blocks are
>definitely lost in loss record 745 of 792
>   at 0x4C2A6D0: calloc (in vgpreload_memcheck-amd64-linux.so)
>   by 0x4E9643D: virAllocVar (viralloc.c:558)
>   by 0x4ED2425: virObjectNew (virobject.c:190)
>   by 0x4F675AC: virGetConnect (datatypes.c:116)
>   by 0x4F6EA06: do_open (libvirt.c:1136)
>   by 0x4F71017: virConnectOpenAuth (libvirt.c:1481)
>   by 0x129FFA: vshReconnect (virsh.c:337)
>   by 0x128310: main (virsh.c:2470)
> ---
>  src/libvirt.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/libvirt.c b/src/libvirt.c
> index 90608ab..4beb40a 100644
> --- a/src/libvirt.c
> +++ b/src/libvirt.c
> @@ -21452,6 +21452,7 @@ int virConnectRegisterCloseCallback(virConnectPtr 
> conn,
>  goto error;
>  }
>  
> +conn->closeCallback->conn = conn;
>  conn->closeCallback->callback = cb;
>  conn->closeCallback->opaque = opaque;
>  conn->closeCallback->freeCallback = freecb;
> 

ACK

Michal

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

[libvirt] [PATCH] Properly unref a connection with a close callback

2013-11-14 Thread Ján Tomko

The connection pointer in the closeCallback data was never
initialized, making the unref in remoteClientCloseFunc a no-op.

This fixes the following leak in virsh when the daemon closes
the connection unexpectedly:

1,179 (288 direct, 891 indirect) bytes in 1 blocks are
   definitely lost in loss record 745 of 792
  at 0x4C2A6D0: calloc (in vgpreload_memcheck-amd64-linux.so)
  by 0x4E9643D: virAllocVar (viralloc.c:558)
  by 0x4ED2425: virObjectNew (virobject.c:190)
  by 0x4F675AC: virGetConnect (datatypes.c:116)
  by 0x4F6EA06: do_open (libvirt.c:1136)
  by 0x4F71017: virConnectOpenAuth (libvirt.c:1481)
  by 0x129FFA: vshReconnect (virsh.c:337)
  by 0x128310: main (virsh.c:2470)
---
 src/libvirt.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/libvirt.c b/src/libvirt.c
index 90608ab..4beb40a 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -21452,6 +21452,7 @@ int virConnectRegisterCloseCallback(virConnectPtr conn,
 goto error;
 }
 
+conn->closeCallback->conn = conn;
 conn->closeCallback->callback = cb;
 conn->closeCallback->opaque = opaque;
 conn->closeCallback->freeCallback = freecb;
-- 
1.8.3.2

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

Re: [libvirt] [PATCHv2] maint: Kill usage of atoi()

2013-11-14 Thread Eric Blake

On 11/14/2013 09:48 AM, Peter Krempa wrote:
> Kill the use of atoi() and introduce syntax check to forbit it and it's

s/forbit/forbid/

> friends (atol, atoll, atof, atoq).
> 
> Also fix a typo in variable name holding the cylinders count of a disk
> pool (apparently unused).
> ---
>  cfg.mk |  9 +
>  src/conf/storage_conf.h|  2 +-
>  src/qemu/qemu_command.c|  8 ++--
>  src/storage/storage_backend_disk.c | 14 +-
>  src/xen/xend_internal.c| 17 +
>  5 files changed, 38 insertions(+), 12 deletions(-)
> 
> diff --git a/cfg.mk b/cfg.mk

> @@ -1040,3 +1046,6 @@ exclude_file_name_regexp--sc_prohibit_int_ijk = \
> 
>  exclude_file_name_regexp--sc_prohibit_getenv = \
>^tests/.*\.[ch]$$
> +
> +exclude_file_name_regexp--sc_prohibit_atoi= \
> + ^examples/.*\.[ch]$$

Drop this hunk.  None of our examples/* use atoi to begin with, so
there's no reason to exclude them from the syntax check.

ACK with those fixes.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



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

Re: [libvirt] [PATCH 6/6] qemuProcessReconnectHelper: Don't create joinable thread

2013-11-14 Thread Michal Privoznik

On 14.11.2013 12:55, Martin Kletzander wrote:
> On Thu, Nov 14, 2013 at 10:51:16AM +0100, Michal Privoznik wrote:
>> In the qemuProcessReconnectHelper() a new thread that do all the
> 
> s/do/does/
> 
>> interesting work is spawned. The rationale is to not block the daemon
>> startup process in case of unresponsive qemu. However, the thread
>> handler is a local variable which gets lost once the control goes out of
>> scope. Hence, the thread gets leaked. We can avoid this if the thread
> 
> s/,// ???
> 
>> isn't make joinable.
>>
> 
> s/make/made/
> 
>> Signed-off-by: Michal Privoznik 
>> ---
>>  src/qemu/qemu_process.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
> 
> ACK,
> 
> Martin

Thanks. I've pushed the patches except for 4/6 which we doesn't have a
clear consensus on yet.

Michal

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

[libvirt] [PATCHv2] maint: Kill usage of atoi()

2013-11-14 Thread Peter Krempa

Kill the use of atoi() and introduce syntax check to forbit it and it's
friends (atol, atoll, atof, atoq).

Also fix a typo in variable name holding the cylinders count of a disk
pool (apparently unused).
---
 cfg.mk |  9 +
 src/conf/storage_conf.h|  2 +-
 src/qemu/qemu_command.c|  8 ++--
 src/storage/storage_backend_disk.c | 14 +-
 src/xen/xend_internal.c| 17 +
 5 files changed, 38 insertions(+), 12 deletions(-)

diff --git a/cfg.mk b/cfg.mk
index befd231..901e307 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -869,6 +869,12 @@ sc_prohibit_getenv:
halt='Use virGetEnv{Allow,Block}SUID instead of getenv' \
  $(_sc_search_regexp)

+sc_prohibit_atoi:
+   @prohibit='\bato(i|f|l|ll|q) *\('   \
+   halt='Use virStrToLong* instead of atoi, atol, atof, atoq, atoll' \
+ $(_sc_search_regexp)
+
+
 # We don't use this feature of maint.mk.
 prev_version_file = /dev/null

@@ -1040,3 +1046,6 @@ exclude_file_name_regexp--sc_prohibit_int_ijk = \

 exclude_file_name_regexp--sc_prohibit_getenv = \
   ^tests/.*\.[ch]$$
+
+exclude_file_name_regexp--sc_prohibit_atoi= \
+   ^examples/.*\.[ch]$$
diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h
index f062bd8..05c291e 100644
--- a/src/conf/storage_conf.h
+++ b/src/conf/storage_conf.h
@@ -227,7 +227,7 @@ struct _virStoragePoolSourceDevice {
  * the geometry data is needed
  */
 struct _geometry {
-int cyliders;
+int cylinders;
 int heads;
 int sectors;
 } geometry;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 966aa0d..25beedf 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3663,8 +3663,12 @@ qemuBuildDriveURIString(virConnectPtr conn,
 if (disk->src && virAsprintf(&volimg, "/%s", disk->src) < 0)
 goto cleanup;

-if (disk->hosts->port) {
-port = atoi(disk->hosts->port);
+if (disk->hosts->port &&
+virStrToLong_i(disk->hosts->port, NULL, 0, &port) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("failed to parse network port '%s'"),
+   disk->hosts->port);
+goto cleanup;
 }

 if (disk->hosts->socket &&
diff --git a/src/storage/storage_backend_disk.c 
b/src/storage/storage_backend_disk.c
index 4e53ec5..a7a7d0e 100644
--- a/src/storage/storage_backend_disk.c
+++ b/src/storage/storage_backend_disk.c
@@ -280,12 +280,16 @@ 
virStorageBackendDiskMakePoolGeometry(virStoragePoolObjPtr pool,
   char **const groups,
   void *data ATTRIBUTE_UNUSED)
 {
+virStoragePoolSourceDevicePtr device = &(pool->def->source.devices[0]);
+if (virStrToLong_i(groups[0], NULL, 0, &device->geometry.cylinders) < 0 ||
+virStrToLong_i(groups[1], NULL, 0, &device->geometry.heads) < 0 ||
+virStrToLong_i(groups[2], NULL, 0, &device->geometry.sectors) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("Failed to create disk pool geometry"));
+return -1;
+}

-   pool->def->source.devices[0].geometry.cyliders = atoi(groups[0]);
-   pool->def->source.devices[0].geometry.heads = atoi(groups[1]);
-   pool->def->source.devices[0].geometry.sectors = atoi(groups[2]);
-
-   return 0;
+return 0;
 }

 static int
diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c
index dc57350..771288c 100644
--- a/src/xen/xend_internal.c
+++ b/src/xen/xend_internal.c
@@ -290,10 +290,19 @@ xend_req(int fd, char **content)
 if (STREQ(buffer, "\r\n"))
 break;

-if (istartswith(buffer, "Content-Length: "))
-content_length = atoi(buffer + 16);
-else if (istartswith(buffer, "HTTP/1.1 "))
-retcode = atoi(buffer + 9);
+if (istartswith(buffer, "Content-Length: ")) {
+if (virStrToLong_i(buffer + 16, NULL, 10, &content_length) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("failed to parse Xend response content 
length"));
+return -1;
+}
+} else if (istartswith(buffer, "HTTP/1.1 ")) {
+if (virStrToLong_i(buffer + 9, NULL, 10, &retcode) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("failed to parse Xend response return code"));
+return -1;
+}
+}
 }

 VIR_FREE(buffer);
-- 
1.8.4.3

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

Re: [libvirt] [PATCH] maint: Kill usage of atoi()

2013-11-14 Thread Peter Krempa

On 11/14/13 17:28, Eric Blake wrote:
> On 11/14/2013 09:20 AM, Peter Krempa wrote:
>> Also fix a typo in variable name holding the cylinders count of a disk
>> pool (apparently unused).
> 
> If we're going to kill atoi(), let's also kill atof, atol, atoq, atoll.

Ok, these can be killed free of charge. None of them is used in libvirt
code. I'll add them to the syntax-check rule.

>  Oh, and maint.mk already has a rule that does this; we just have it
> disabled because we aren't clean yet (it also prohibits *scanf, because
> that also has undefined behavior on overflow; but we haven't scrubbed
> our sources to get rid of scanf usage).

There's a ton of *scanf usage which will not be trivial to kill.

> 
>>
>> +sc_prohibit_atoi:
>> +@prohibit='\batoi *\('  
>> \
>> +exclude='exempt from syntax-check'  \
> 
> What are we exempting?  You can probably drop this line.

Right I now figured out what this is used for :).

> 
>> +halt='Use virStrToLong* instead of atoi'\
> 
> This at least is a more specific message than the generic check in maint.mk.
> 

V2 comming soon.

Peter




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

Re: [libvirt] [PATCH] maint: Kill usage of atoi()

2013-11-14 Thread Eric Blake

On 11/14/2013 09:20 AM, Peter Krempa wrote:
> Also fix a typo in variable name holding the cylinders count of a disk
> pool (apparently unused).

If we're going to kill atoi(), let's also kill atof, atol, atoq, atoll.
 Oh, and maint.mk already has a rule that does this; we just have it
disabled because we aren't clean yet (it also prohibits *scanf, because
that also has undefined behavior on overflow; but we haven't scrubbed
our sources to get rid of scanf usage).

> 
> +sc_prohibit_atoi:
> + @prohibit='\batoi *\('  
> \
> + exclude='exempt from syntax-check'  \

What are we exempting?  You can probably drop this line.

> + halt='Use virStrToLong* instead of atoi'\

This at least is a more specific message than the generic check in maint.mk.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org

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

Re: [libvirt] [PATCH] maint: Kill usage of atoi()

2013-11-14 Thread Peter Krempa

On 11/14/13 17:20, Peter Krempa wrote:
> Also fix a typo in variable name holding the cylinders count of a disk
> pool (apparently unused).
> ---
>  cfg.mk | 10 ++
>  src/conf/storage_conf.h|  2 +-
>  src/qemu/qemu_command.c|  8 ++--
>  src/storage/storage_backend_disk.c | 16 +++-
>  src/xen/xend_internal.c| 17 +
>  5 files changed, 41 insertions(+), 12 deletions(-)
> 

...

> diff --git a/src/storage/storage_backend_disk.c 
> b/src/storage/storage_backend_disk.c
> index 4e53ec5..1f30b06 100644
> --- a/src/storage/storage_backend_disk.c
> +++ b/src/storage/storage_backend_disk.c
> @@ -280,12 +280,18 @@ 
> virStorageBackendDiskMakePoolGeometry(virStoragePoolObjPtr pool,
>char **const groups,
>void *data ATTRIBUTE_UNUSED)
>  {
> +pool = pool;
> +groups = groups;

Hmm, I forgot to commit removal of this test statements :/

Consider this squashed in:
diff --git a/src/storage/storage_backend_disk.c 
b/src/storage/storage_backend_disk.c
index 1f30b06..a7a7d0e 100644
--- a/src/storage/storage_backend_disk.c
+++ b/src/storage/storage_backend_disk.c
@@ -280,8 +280,6 @@ virStorageBackendDiskMakePoolGeometry(virStoragePoolObjPtr 
pool,
   char **const groups,
   void *data ATTRIBUTE_UNUSED)
 {
-pool = pool;
-groups = groups;
 virStoragePoolSourceDevicePtr device = &(pool->def->source.devices[0]);
 if (virStrToLong_i(groups[0], NULL, 0, &device->geometry.cylinders) < 0 ||
 virStrToLong_i(groups[1], NULL, 0, &device->geometry.heads) < 0 ||



> +virStoragePoolSourceDevicePtr device = &(pool->def->source.devices[0]);
> +if (virStrToLong_i(groups[0], NULL, 0, &device->geometry.cylinders) < 0 
> ||
> +virStrToLong_i(groups[1], NULL, 0, &device->geometry.heads) < 0 ||
> +virStrToLong_i(groups[2], NULL, 0, &device->geometry.sectors) < 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +   _("Failed to create disk pool geometry"));
> +return -1;
> +}
> 
> -   pool->def->source.devices[0].geometry.cyliders = atoi(groups[0]);
> -   pool->def->source.devices[0].geometry.heads = atoi(groups[1]);
> -   pool->def->source.devices[0].geometry.sectors = atoi(groups[2]);
> -
> -   return 0;
> +return 0;
>  }
> 

Peter




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

[libvirt] [PATCH] maint: Kill usage of atoi()

2013-11-14 Thread Peter Krempa

Also fix a typo in variable name holding the cylinders count of a disk
pool (apparently unused).
---
 cfg.mk | 10 ++
 src/conf/storage_conf.h|  2 +-
 src/qemu/qemu_command.c|  8 ++--
 src/storage/storage_backend_disk.c | 16 +++-
 src/xen/xend_internal.c| 17 +
 5 files changed, 41 insertions(+), 12 deletions(-)

diff --git a/cfg.mk b/cfg.mk
index befd231..4df81d2 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -869,6 +869,13 @@ sc_prohibit_getenv:
halt='Use virGetEnv{Allow,Block}SUID instead of getenv' \
  $(_sc_search_regexp)

+sc_prohibit_atoi:
+   @prohibit='\batoi *\('  
\
+   exclude='exempt from syntax-check'  \
+   halt='Use virStrToLong* instead of atoi'\
+ $(_sc_search_regexp)
+
+
 # We don't use this feature of maint.mk.
 prev_version_file = /dev/null

@@ -1040,3 +1047,6 @@ exclude_file_name_regexp--sc_prohibit_int_ijk = \

 exclude_file_name_regexp--sc_prohibit_getenv = \
   ^tests/.*\.[ch]$$
+
+exclude_file_name_regexp--sc_prohibit_atoi= \
+   ^examples/.*\.[ch]$$
diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h
index f062bd8..05c291e 100644
--- a/src/conf/storage_conf.h
+++ b/src/conf/storage_conf.h
@@ -227,7 +227,7 @@ struct _virStoragePoolSourceDevice {
  * the geometry data is needed
  */
 struct _geometry {
-int cyliders;
+int cylinders;
 int heads;
 int sectors;
 } geometry;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 966aa0d..25beedf 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3663,8 +3663,12 @@ qemuBuildDriveURIString(virConnectPtr conn,
 if (disk->src && virAsprintf(&volimg, "/%s", disk->src) < 0)
 goto cleanup;

-if (disk->hosts->port) {
-port = atoi(disk->hosts->port);
+if (disk->hosts->port &&
+virStrToLong_i(disk->hosts->port, NULL, 0, &port) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("failed to parse network port '%s'"),
+   disk->hosts->port);
+goto cleanup;
 }

 if (disk->hosts->socket &&
diff --git a/src/storage/storage_backend_disk.c 
b/src/storage/storage_backend_disk.c
index 4e53ec5..1f30b06 100644
--- a/src/storage/storage_backend_disk.c
+++ b/src/storage/storage_backend_disk.c
@@ -280,12 +280,18 @@ 
virStorageBackendDiskMakePoolGeometry(virStoragePoolObjPtr pool,
   char **const groups,
   void *data ATTRIBUTE_UNUSED)
 {
+pool = pool;
+groups = groups;
+virStoragePoolSourceDevicePtr device = &(pool->def->source.devices[0]);
+if (virStrToLong_i(groups[0], NULL, 0, &device->geometry.cylinders) < 0 ||
+virStrToLong_i(groups[1], NULL, 0, &device->geometry.heads) < 0 ||
+virStrToLong_i(groups[2], NULL, 0, &device->geometry.sectors) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("Failed to create disk pool geometry"));
+return -1;
+}

-   pool->def->source.devices[0].geometry.cyliders = atoi(groups[0]);
-   pool->def->source.devices[0].geometry.heads = atoi(groups[1]);
-   pool->def->source.devices[0].geometry.sectors = atoi(groups[2]);
-
-   return 0;
+return 0;
 }

 static int
diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c
index dc57350..771288c 100644
--- a/src/xen/xend_internal.c
+++ b/src/xen/xend_internal.c
@@ -290,10 +290,19 @@ xend_req(int fd, char **content)
 if (STREQ(buffer, "\r\n"))
 break;

-if (istartswith(buffer, "Content-Length: "))
-content_length = atoi(buffer + 16);
-else if (istartswith(buffer, "HTTP/1.1 "))
-retcode = atoi(buffer + 9);
+if (istartswith(buffer, "Content-Length: ")) {
+if (virStrToLong_i(buffer + 16, NULL, 10, &content_length) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("failed to parse Xend response content 
length"));
+return -1;
+}
+} else if (istartswith(buffer, "HTTP/1.1 ")) {
+if (virStrToLong_i(buffer + 9, NULL, 10, &retcode) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("failed to parse Xend response return code"));
+return -1;
+}
+}
 }

 VIR_FREE(buffer);
-- 
1.8.4.3

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

Re: [libvirt] [PATCHv3 3/3] Return -1 in virPortAllocatorAcquire if all ports are used

2013-11-14 Thread Martin Kletzander

On Wed, Nov 13, 2013 at 06:01:42PM +0100, Ján Tomko wrote:
> Report the error in virPortAllocatorAcquire instead
> of doing it in every caller.
> 
> The error contains the port range name instead of the intended
> use for the port, e.g.:
> Unable to find an unused port in range 'display' (65534-65535)
> instead of:
> Unable to find an unused port for SPICE
> 
> This also adds error reporting when the QEMU driver could not
> find an unused port for VNC, VNC WebSockets or NBD migration.
> ---
>  src/libxl/libxl_conf.c   |  5 -
>  src/qemu/qemu_migration.c| 16 ++--
>  src/qemu/qemu_process.c  | 11 ---
>  src/util/virportallocator.c  |  7 ++-
>  tests/virportallocatortest.c |  6 ++
>  5 files changed, 10 insertions(+), 35 deletions(-)
> 

ACK,

Martin


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

[libvirt] [PATCH not-for-inclusion] accomodate new qemu migration status 'setup'

2013-11-14 Thread Serge Hallyn

Hi,

newer qemu introduces a new migration state 'setup', which libvirt does
not yet know about.  This upsets our qa-regression-tests :)  I know
Eric is working on a fix but is short on time, so I thought I would send
the patch that is working for me.  I don't expect it to be a proper fix,
as I suspect the proper fix should actually add a new migration state in
libvirt itself.  But it appears to be working.

This patch treats the new setup state as as active, but doesn't try to
query for status info yet.

Signed-off-by: Serge Hallyn 
---
 src/qemu/qemu_migration.c| 48 
 src/qemu/qemu_monitor.c  |  8 +---
 src/qemu/qemu_monitor.h  |  3 ++-
 src/qemu/qemu_monitor_json.c |  8 +++-
 src/qemu/qemu_monitor_text.c |  9 -
 5 files changed, 53 insertions(+), 23 deletions(-)

Index: libvirt-1.1.4/src/qemu/qemu_migration.c
===
--- libvirt-1.1.4.orig/src/qemu/qemu_migration.c2013-11-13 
20:47:05.100085000 +0100
+++ libvirt-1.1.4/src/qemu/qemu_migration.c 2013-11-13 20:47:05.092085000 
+0100
@@ -1614,7 +1614,7 @@
  enum qemuDomainAsyncJob asyncJob)
 {
 qemuDomainObjPrivatePtr priv = vm->privateData;
-int ret;
+int ret, setting_up;
 qemuMonitorMigrationStatus status;
 
 memset(&status, 0, sizeof(status));
@@ -1624,7 +1624,7 @@
 /* Guest already exited; nothing further to update.  */
 return -1;
 }
-ret = qemuMonitorGetMigrationStatus(priv->mon, &status);
+ret = qemuMonitorGetMigrationStatus(priv->mon, &status, &setting_up);
 
 qemuDomainObjExitMonitor(driver, vm);
 
@@ -1645,21 +1645,35 @@
 break;
 
 case QEMU_MONITOR_MIGRATION_STATUS_ACTIVE:
-priv->job.info.fileTotal = priv->job.status.disk_total;
-priv->job.info.fileRemaining = priv->job.status.disk_remaining;
-priv->job.info.fileProcessed = priv->job.status.disk_transferred;
-
-priv->job.info.memTotal = priv->job.status.ram_total;
-priv->job.info.memRemaining = priv->job.status.ram_remaining;
-priv->job.info.memProcessed = priv->job.status.ram_transferred;
-
-priv->job.info.dataTotal =
-priv->job.status.ram_total + priv->job.status.disk_total;
-priv->job.info.dataRemaining =
-priv->job.status.ram_remaining + priv->job.status.disk_remaining;
-priv->job.info.dataProcessed =
-priv->job.status.ram_transferred +
-priv->job.status.disk_transferred;
+if (setting_up) {
+priv->job.info.fileTotal = -1;
+priv->job.info.fileRemaining = -1;
+priv->job.info.fileProcessed = 0;
+
+priv->job.info.memTotal = -1;
+priv->job.info.memRemaining = -1;
+priv->job.info.memProcessed = 0;
+
+priv->job.info.dataTotal = -1;
+priv->job.info.dataRemaining = -1;
+priv->job.info.dataProcessed = 0;
+} else {
+priv->job.info.fileTotal = priv->job.status.disk_total;
+priv->job.info.fileRemaining = priv->job.status.disk_remaining;
+priv->job.info.fileProcessed = priv->job.status.disk_transferred;
+
+priv->job.info.memTotal = priv->job.status.ram_total;
+priv->job.info.memRemaining = priv->job.status.ram_remaining;
+priv->job.info.memProcessed = priv->job.status.ram_transferred;
+
+priv->job.info.dataTotal =
+priv->job.status.ram_total + priv->job.status.disk_total;
+priv->job.info.dataRemaining =
+priv->job.status.ram_remaining + 
priv->job.status.disk_remaining;
+priv->job.info.dataProcessed =
+priv->job.status.ram_transferred +
+priv->job.status.disk_transferred;
+}
 
 ret = 0;
 break;
Index: libvirt-1.1.4/src/qemu/qemu_monitor.c
===
--- libvirt-1.1.4.orig/src/qemu/qemu_monitor.c  2013-11-13 20:47:05.100085000 
+0100
+++ libvirt-1.1.4/src/qemu/qemu_monitor.c   2013-11-13 20:57:06.932085000 
+0100
@@ -2087,7 +2087,8 @@
 
 
 int qemuMonitorGetMigrationStatus(qemuMonitorPtr mon,
-  qemuMonitorMigrationStatusPtr status)
+  qemuMonitorMigrationStatusPtr status,
+  int *setting_up)
 {
 int ret;
 VIR_DEBUG("mon=%p", mon);
@@ -2098,10 +2099,11 @@
 return -1;
 }
 
+*setting_up = 0;
 if (mon->json)
-ret = qemuMonitorJSONGetMigrationStatus(mon, status);
+ret = qemuMonitorJSONGetMigrationStatus(mon, status, setting_up);
 else
-ret = qemuMonitorTextGetMigrationStatus(mon, status);
+ret = qemuMonitorTextGetMigrationStatus(mon, status, setting_up);
 return ret;
 }
 
Index: libvirt-1.1.4/src/qemu/qemu_monitor.h
===

Re: [libvirt] [PATCH] qemu: Call qemuSetupHostdevCGroup later during hotplug

2013-11-14 Thread Daniel P. Berrange

On Thu, Nov 14, 2013 at 03:04:13PM +0100, Jiri Denemark wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1025108
> 
> So far qemuSetupHostdevCGroup was called very early during hotplug, even
> before we knew the device we were about to hotplug was actually
> available. By calling the function later, we make sure QEMU won't be
> allowed to access devices used by other domains.
> 
> Another important effect of this change is that hopluging USB devices
> specified by vendor and product (but not by their USB address) works
> again. This was broken since v1.0.5-171-g7d763ac, when the call to
> qemuFindHostdevUSBDevice was moved after the call to
> qemuSetupHostdevCGroup, which then used an uninitialized USB address.
> 
> Signed-off-by: Jiri Denemark 
> ---
>  src/qemu/qemu_hotplug.c | 36 +++-
>  1 file changed, 27 insertions(+), 9 deletions(-)

I won't nack the patch for this, but wanted to point out that we should
expand the qemuhotplugtest.c file to use an LD_PRELOAD to mock out the
cgroups filesystem. Then we can validate that the cgroups setup is being
done correctly for each hotplug operation we test.

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

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

Re: [libvirt] [PATCH 4/6] virThreadPoolFree: Join worker threads

2013-11-14 Thread Daniel P. Berrange

On Thu, Nov 14, 2013 at 07:13:43AM -0700, Eric Blake wrote:
> On 11/14/2013 02:51 AM, Michal Privoznik wrote:
> > Even though currently we are freeing the pool of worker threads at the
> > daemon very end, nothing holds us back in joining the worker threads.
> 
> Would it be better to detach the threads than to wait for them to join?

There's code earlier which uses a condition variable wait for them to
actually exit, so IIUC, joining them shouldn't add any delay.

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

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

Re: [libvirt] [PATCH 4/6] virThreadPoolFree: Join worker threads

2013-11-14 Thread Eric Blake

On 11/14/2013 02:51 AM, Michal Privoznik wrote:
> Even though currently we are freeing the pool of worker threads at the
> daemon very end, nothing holds us back in joining the worker threads.

Would it be better to detach the threads than to wait for them to join?

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



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

[libvirt] [PATCH] qemu: Call qemuSetupHostdevCGroup later during hotplug

2013-11-14 Thread Jiri Denemark

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

So far qemuSetupHostdevCGroup was called very early during hotplug, even
before we knew the device we were about to hotplug was actually
available. By calling the function later, we make sure QEMU won't be
allowed to access devices used by other domains.

Another important effect of this change is that hopluging USB devices
specified by vendor and product (but not by their USB address) works
again. This was broken since v1.0.5-171-g7d763ac, when the call to
qemuFindHostdevUSBDevice was moved after the call to
qemuSetupHostdevCGroup, which then used an uninitialized USB address.

Signed-off-by: Jiri Denemark 
---
 src/qemu/qemu_hotplug.c | 36 +++-
 1 file changed, 27 insertions(+), 9 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 6eb483c..0d9a3aa 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1135,6 +1135,7 @@ int qemuDomainAttachHostPciDevice(virQEMUDriverPtr driver,
 int configfd = -1;
 char *configfd_name = NULL;
 bool releaseaddr = false;
+bool teardowncgroup = false;
 int backend = hostdev->source.subsys.u.pci.backend;
 
 if (VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs + 1) < 0)
@@ -1171,6 +1172,10 @@ int qemuDomainAttachHostPciDevice(virQEMUDriverPtr 
driver,
 break;
 }
 
+if (qemuSetupHostdevCGroup(vm, hostdev) < 0)
+goto error;
+teardowncgroup = true;
+
 if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
 if (qemuAssignDeviceHostdevAlias(vm->def, hostdev, -1) < 0)
 goto error;
@@ -1226,6 +1231,9 @@ int qemuDomainAttachHostPciDevice(virQEMUDriverPtr driver,
 return 0;
 
 error:
+if (teardowncgroup && qemuTeardownHostdevCgroup(vm, hostdev) < 0)
+VIR_WARN("Unable to remove host device cgroup ACL on hotplug fail");
+
 if (releaseaddr)
 qemuDomainReleaseDeviceAddress(vm, hostdev->info, NULL);
 
@@ -1418,6 +1426,7 @@ int qemuDomainAttachHostUsbDevice(virQEMUDriverPtr driver,
 virUSBDevicePtr usb = NULL;
 char *devstr = NULL;
 bool added = false;
+bool teardowncgroup = false;
 int ret = -1;
 
 if (qemuFindHostdevUSBDevice(hostdev, true, &usb) < 0)
@@ -1435,6 +1444,10 @@ int qemuDomainAttachHostUsbDevice(virQEMUDriverPtr 
driver,
 added = true;
 virUSBDeviceListSteal(list, usb);
 
+if (qemuSetupHostdevCGroup(vm, hostdev) < 0)
+goto cleanup;
+teardowncgroup = true;
+
 if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
 if (qemuAssignDeviceHostdevAlias(vm->def, hostdev, -1) < 0)
 goto cleanup;
@@ -1461,6 +1474,10 @@ int qemuDomainAttachHostUsbDevice(virQEMUDriverPtr 
driver,
 
 ret = 0;
 cleanup:
+if (ret < 0 &&
+teardowncgroup &&
+qemuTeardownHostdevCgroup(vm, hostdev) < 0)
+VIR_WARN("Unable to remove host device cgroup ACL on hotplug fail");
 if (added)
 virUSBDeviceListSteal(driver->activeUsbHostdevs, usb);
 virUSBDeviceFree(usb);
@@ -1478,6 +1495,7 @@ qemuDomainAttachHostScsiDevice(virQEMUDriverPtr driver,
 qemuDomainObjPrivatePtr priv = vm->privateData;
 char *devstr = NULL;
 char *drvstr = NULL;
+bool teardowncgroup = false;
 
 if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE) ||
 !virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE) ||
@@ -1498,6 +1516,10 @@ qemuDomainAttachHostScsiDevice(virQEMUDriverPtr driver,
 return -1;
 }
 
+if (qemuSetupHostdevCGroup(vm, hostdev) < 0)
+goto cleanup;
+teardowncgroup = true;
+
 if (qemuAssignDeviceHostdevAlias(vm->def, hostdev, -1) < 0)
 goto cleanup;
 
@@ -1535,8 +1557,11 @@ qemuDomainAttachHostScsiDevice(virQEMUDriverPtr driver,
 
 ret = 0;
 cleanup:
-if (ret < 0)
+if (ret < 0) {
 qemuDomainReAttachHostScsiDevices(driver, vm->def->name, &hostdev, 1);
+if (teardowncgroup && qemuTeardownHostdevCgroup(vm, hostdev) < 0)
+VIR_WARN("Unable to remove host device cgroup ACL on hotplug 
fail");
+}
 VIR_FREE(drvstr);
 VIR_FREE(devstr);
 return ret;
@@ -1553,12 +1578,9 @@ int qemuDomainAttachHostDevice(virQEMUDriverPtr driver,
 return -1;
 }
 
-if (qemuSetupHostdevCGroup(vm, hostdev) < 0)
-return -1;
-
 if (virSecurityManagerSetHostdevLabel(driver->securityManager,
   vm->def, hostdev, NULL) < 0)
-goto cleanup;
+return -1;
 
 switch (hostdev->source.subsys.type) {
 case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI:
@@ -1592,10 +1614,6 @@ error:
 if (virSecurityManagerRestoreHostdevLabel(driver->securityManager,
   vm->def, hostdev, NULL) < 0)
 VIR_WARN("Unable to restore host device labelling on hotplug fail");
-
-cleanup:
-if (qemuTeardownHostdevCgroup(vm, hostdev) < 0)
-VIR_WARN("Unable to remove host device cgroup ACL on h

Re: [libvirt] [PATCH v2] virsh domxml-from-native to treat SCSI as the bus type for pseries by default

2013-11-14 Thread Cole Robinson

On 11/14/2013 04:11 AM, Shivaprasad bhat wrote:
> Thanks Cole.
> 
> The change is to correct the IDE disk type to SCSI on pseries systems for
> domxml-from-native.
> 
> Here is the test case and results. 
> 
> sh# cat cmd.txt
> *qemu-system-ppc64 -M pseries -m 4096 -nographic -enable-kvm -hda
> /data/images/rhel70.qcow2 -name rhel70 -cdrom
> /data/iso/RHEL-7.0-20130306.0-Server-ppc64-dvd1.iso -boot d -vnc :30*
> 
> sh# ./run tools/*virsh domxml-from-native qemu-argv cmd.txt*
> 
>   rhel70
>   eae019ae-a155-4dd8-be21-f9738b6aedea
>   4194304
>   4194304
>   1
>   
> hvm
> 
>   
>   
>   destroy
>   restart
>   destroy
>   
> qemu-system-ppc64
> 
>   
>   
> *  //> ide is
> changed to SCSI*
>   
> 
> 
>   
>   
> *   //> ide is changed
> to SCSI*
>   
>   
> 
>   *   //> Controller ide
> also changed to SCSI*
> 
> 
> 
> 
>   
> 
> 
>   
> 
> 
>   
> 
> sh# 
> 

Thanks, but what I meant was to add an automated test case under the tests/
directory that demonstrates what is actually fixed. See
tests/qemuargv2xmltest.c, will require appropriate *.args and *.xml files in
tests/qemuxml2argvdata/

- Cole

> 
> 
> On Thu, Nov 14, 2013 at 3:02 AM, Cole Robinson  > wrote:
> 
> On 11/13/2013 04:31 PM, Shivaprasad bhat wrote:
> > Hi Jan, Cole,
> >
> > Could you please reviewing my patch ?
> >
> > Thanks,
> > Shiva
> >
> 
> I'd recommend adding a test case that demonstrates what exactly this is
> changing.
> 
> - Cole
> 
> >
> > On Wed, Oct 30, 2013 at 1:37 PM, Shivaprasad bhat
> mailto:shivaprasadb...@gmail.com>
> > >>
> wrote:
> >
> > Hi,
> >
> > Could someone please help reviewing the patch ?
> >
> > Thanks and Regards,
> > Shiva
> >
> >
> > On Mon, Oct 28, 2013 at 2:50 PM, Shivaprasad G Bhat
> > mailto:shivaprasadb...@gmail.com>
> >> 
> wrote:
> >
> > The bus type IDE being enum Zero, the bus type on pseries system
> > appears as IDE for all the disk types. Pseries platform needs
> this to
> > appear as SCSI instead of IDE.
> >
> > Signed-off-by: Shivaprasad G Bhat  
> >  >>
> > ---
> >  src/qemu/qemu_domain.c |   11 +++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > index b8aec2d..df06c13 100644
> > --- a/src/qemu/qemu_domain.c
> > +++ b/src/qemu/qemu_domain.c
> > @@ -827,6 +827,12 @@
> > qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
> >  if (dev->type == VIR_DOMAIN_DEVICE_DISK) {
> >  virDomainDiskDefPtr disk = dev->data.disk;
> >
> > +if ((def->os.arch == VIR_ARCH_PPC64) &&
> > +def->os.machine && STREQ(def->os.machine, 
> "pseries") &&
> > +(disk->bus == VIR_DOMAIN_DISK_BUS_IDE)) {
> > +disk->bus = VIR_DOMAIN_DISK_BUS_SCSI;
> > +}
> > +
> >  /* both of these require data from the driver config */
> >  if (driver && (cfg = virQEMUDriverGetConfig(driver))) {
> >  /* assign default storage format and driver
> according to
> > config */
> > @@ -868,6 +874,11 @@
> > qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
> >  (def->os.arch == VIR_ARCH_S390 || def->os.arch ==
> > VIR_ARCH_S390X))
> >  dev->data.chr->targetType =
> > VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO;
> >
> > +if (dev->type == VIR_DOMAIN_DEVICE_CONTROLLER &&
> > +dev->data.controller->type ==
> VIR_DOMAIN_CONTROLLER_TYPE_IDE &&
> > +def->os.machine && STREQ(def->os.machine, "pseries"))
> > +dev->data.controller->type =
> VIR_DOMAIN_CONTROLLER_TYPE_SCSI;
> > +
> >  /* set the default USB model to none for s390 unless an 
> address
> > is found */
> >  if (dev->type == VIR_DOMAIN_DEVICE_CONTROLLER &&
> >  dev->data.controller->type ==
> VIR_DOMAIN_CONTROLLER_TYPE_USB &&
> >
> > --
> > libvir-list mailing list
> > libvir-list@redhat.com 
>

Re: [libvirt] [PATCHv3 2/3] Add a name to virPortAllocator

2013-11-14 Thread Martin Kletzander

On Wed, Nov 13, 2013 at 06:01:41PM +0100, Ján Tomko wrote:
> This allows its error messages to be more specific.
> ---
>  src/libxl/libxl_driver.c | 3 ++-
>  src/qemu/qemu_driver.c   | 9 ++---
>  src/util/virportallocator.c  | 9 +++--
>  src/util/virportallocator.h  | 3 ++-
>  tests/virportallocatortest.c | 4 ++--
>  5 files changed, 19 insertions(+), 9 deletions(-)
> 

You leak the *name.  ACK if you add VIR_FREE(pa->name) into
virPortAllocatorDispose().

Martin


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

Re: [libvirt] [PATCHv3 1/3] Don't release spice port twice when no TLS port is available

2013-11-14 Thread Martin Kletzander

On Wed, Nov 13, 2013 at 06:01:40PM +0100, Ján Tomko wrote:
> Introduced by 7b4a630.
> ---
>  src/qemu/qemu_process.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index e34f542..1365b59 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -3431,7 +3431,6 @@ qemuProcessSPICEAllocatePorts(virQEMUDriverPtr driver,
>  if (tlsPort == 0) {
>  virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("Unable to find an unused port for SPICE 
> TLS"));
> -virPortAllocatorRelease(driver->remotePorts, port);
>  goto error;
>  }
>  graphics->data.spice.tlsPort = tlsPort;
> -- 
> 1.8.3.2
> 

ACK,

Martin


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

Re: [libvirt] [Qemu-devel] -vga std vs. -device VGA

2013-11-14 Thread Eric Blake

[adding libvirt]

On 11/13/2013 10:01 PM, Alexey Kardashevskiy wrote:
> Hi everyone.
> 
> Here is a problem with the SPAPR machine and a libvirt's habit to use
> "-nodefaults".
> 
> When we run QEMU with "-vga std", a VGA device is created from the
> machine_init callback and if VGA is added, then we automatically add a PCI
> USB OHCI adapter with a keyboard and everybody (SLOF, yaboot, guest kernel)
> is happy - there are both input and output devices.
> 
> However libvirt uses "-device VGA" to create a VGA device. In this case the
> actual VGA device is created in vl.c and we (SPAPR) do not have control
> over this process so we do not automatically create anything in addition to
> VGA. As a result, yaboot fails as there is no input device.
> 
> x86 creates a whole bunch of various devices including a keyboard
> controller (i8042) even with "-nodefaults" but this is not true for SPAPR.

Then libvirt needs to be taught to provide the same devices via -device
that would be present during -vga.

> 
> Since we (POWERPC folks) want everything to work as close to x86 as
> possible, we need to do something about it :)

Hmm, but adding automatic devices in a future qemu release compared to
what you do now could be a regression - once libvirt is taught how to
provide the correct devices for the qemu that exists now, wouldn't that
mean that your automatic device adding creates duplicates?

Libvirt uses -nodefaults for a reason - we really don't like automatic
devices (especially when the set of WHAT is automatic is prone to change
between qemu builds), and would much rather call out everything
explicitly so that we can guarantee guest ABI stability across qemu
upgrades.

> 
> So the question is - is there any proper (i. e. qemu-upstreamable) way to
> detect a VGA device presence and create additional devices (OHCI, keyboard,
> mouse) as "-vga" does it now? The machine reset callback is too late for that.
> 
> Or we should just print an error (in QEMU or SLOF) and do nothing and let
> the user configure all required devices in libvirt?

I'm in favor of printing an error if not enough devices were supplied,
and fixing libvirt to supply the correct devices when it doesn't use -vga.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org

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

Re: [libvirt] [PATCH 6/6] qemuProcessReconnectHelper: Don't create joinable thread

2013-11-14 Thread Martin Kletzander

On Thu, Nov 14, 2013 at 10:51:16AM +0100, Michal Privoznik wrote:
> In the qemuProcessReconnectHelper() a new thread that do all the

s/do/does/

> interesting work is spawned. The rationale is to not block the daemon
> startup process in case of unresponsive qemu. However, the thread
> handler is a local variable which gets lost once the control goes out of
> scope. Hence, the thread gets leaked. We can avoid this if the thread

s/,// ???

> isn't make joinable.
> 

s/make/made/

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

ACK,

Martin


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

Re: [libvirt] [PATCH 3/6] virPCIDeviceBindToStub: Remove unused @oldDriverPath and @oldDriverName

2013-11-14 Thread Martin Kletzander

On Thu, Nov 14, 2013 at 10:51:13AM +0100, Michal Privoznik wrote:
> These two chunks had to be part of df4283a55bf. But for some unclear
> reason, the weren't. Anyway, these two variables are not used anywhere
> within function. They're initialized to NULL and then VIR_FREE()-d. And
> there's no reason do do two NOPs, right?
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/util/virpci.c | 4 
>  1 file changed, 4 deletions(-)
> 

ACK,

Martin


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

Re: [libvirt] [PATCH 5/6] virDomainEventCallbackListFree: Don't leak @list->callbacks

2013-11-14 Thread Martin Kletzander

On Thu, Nov 14, 2013 at 10:51:15AM +0100, Michal Privoznik wrote:
> The @list->callbacks is an array that is inflated whenever a new event
> is added, e.g. via virDomainEventCallbackListAddID(). However, when we
> are freeing the array, we free the items within it but forgot to
> actually free it.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/conf/domain_event.c | 1 +
>  1 file changed, 1 insertion(+)
> 

ACK,

Martin


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

Re: [libvirt] [PATCH 4/6] virThreadPoolFree: Join worker threads

2013-11-14 Thread Martin Kletzander

On Thu, Nov 14, 2013 at 10:51:14AM +0100, Michal Privoznik wrote:
> Even though currently we are freeing the pool of worker threads at the
> daemon very end, nothing holds us back in joining the worker threads.
> 

s/daemon/daemon's/ ???

Unless the thread is blocked, right?  Not that it should happen, but
maybe adding a wrapper for pthread_timedjoin_np and using that (if
available) might be better?  Just an idea, I haven't thought that
through ;)

> Signed-off-by: Michal Privoznik 
> ---
>  src/util/virthreadpool.c | 9 +
>  1 file changed, 9 insertions(+)
> 
> diff --git a/src/util/virthreadpool.c b/src/util/virthreadpool.c
> index b1e2c0c..99f36ec 100644
> --- a/src/util/virthreadpool.c
> +++ b/src/util/virthreadpool.c
> @@ -241,6 +241,9 @@ void virThreadPoolFree(virThreadPoolPtr pool)
>  {
>  virThreadPoolJobPtr job;
>  bool priority = false;
> +size_t i;
> +size_t nWorkers = pool->nWorkers;
> +size_t nPrioWorkers = pool->nPrioWorkers;
>  
>  if (!pool)
>  return;
> @@ -262,6 +265,12 @@ void virThreadPoolFree(virThreadPoolPtr pool)
>  VIR_FREE(job);
>  }
>  
> +for (i = 0; i < nWorkers; i++)
> +virThreadJoin(&pool->workers[i]);
> +
> +for (i = 0; i < nPrioWorkers; i++)
> +virThreadJoin(&pool->prioWorkers[i]);
> +
>  VIR_FREE(pool->workers);
>  virMutexUnlock(&pool->mutex);
>  virMutexDestroy(&pool->mutex);
> -- 
> 1.8.3.2
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list


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

Re: [libvirt] [PATCH 2/6] networkBuildDhcpDaemonCommandLine: Don't leak @configfile

2013-11-14 Thread Martin Kletzander

On Thu, Nov 14, 2013 at 10:51:12AM +0100, Michal Privoznik wrote:
> The name of the dnsmasq's config file is initialized and passed to
> virCommand. However, the virCommand creates a copy, so we are
> responsible for freeing the @configfile ourselves.
> 

Squashing with 1/1 would've been fine too ;)

ACK,

Martin


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

Re: [libvirt] [PATCH 1/6] networkBuildDhcpDaemonCommandLine: Don't leak @configstr

2013-11-14 Thread Martin Kletzander

On Thu, Nov 14, 2013 at 10:51:11AM +0100, Michal Privoznik wrote:
> In the function an helper to build the dnsmasq config file is used. The
> content to be stored then into the config file is kept in @configstr
> variable. However, once written, it's never freed and as soon as the
> control reaches the 'return' line, the variable (and hence allocated
> memory handle) will go off the scope and is leaked.
> 

Vary hard to parse, I'd say $SUBJ is enough.

> Signed-off-by: Michal Privoznik 
> ---
>  src/network/bridge_driver.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 3423a45..fd4dfc9 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -1061,6 +1061,7 @@ networkBuildDhcpDaemonCommandLine(virNetworkObjPtr 
> network,
>  *cmdout = cmd;
>  ret = 0;
>  cleanup:
> +VIR_FREE(configstr);
>  return ret;
>  }
>  
> -- 
> 1.8.3.2
> 

ACK,

Martin


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

Re: [libvirt] [PATCH v2] virsh domxml-from-native to treat SCSI as the bus type for pseries by default

2013-11-14 Thread Ján Tomko

On 10/28/2013 10:20 AM, Shivaprasad G Bhat wrote:
> The bus type IDE being enum Zero, the bus type on pseries system appears as 
> IDE for all the disk types. Pseries platform needs this to appear as SCSI 
> instead of IDE.
> 
> Signed-off-by: Shivaprasad G Bhat 
> ---
>  src/qemu/qemu_domain.c |   11 +++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index b8aec2d..df06c13 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -827,6 +827,12 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
>  if (dev->type == VIR_DOMAIN_DEVICE_DISK) {
>  virDomainDiskDefPtr disk = dev->data.disk;
>  
> +if ((def->os.arch == VIR_ARCH_PPC64) &&
> +def->os.machine && STREQ(def->os.machine, "pseries") &&
> +(disk->bus == VIR_DOMAIN_DISK_BUS_IDE)) {
> +disk->bus = VIR_DOMAIN_DISK_BUS_SCSI;
> +}
> +
>  /* both of these require data from the driver config */
>  if (driver && (cfg = virQEMUDriverGetConfig(driver))) {
>  /* assign default storage format and driver according to config 
> */
> @@ -868,6 +874,11 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
>  (def->os.arch == VIR_ARCH_S390 || def->os.arch == VIR_ARCH_S390X))
>  dev->data.chr->targetType = 
> VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO;
>  
> +if (dev->type == VIR_DOMAIN_DEVICE_CONTROLLER &&
> +dev->data.controller->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE &&
> +def->os.machine && STREQ(def->os.machine, "pseries"))
> +dev->data.controller->type = VIR_DOMAIN_CONTROLLER_TYPE_SCSI;
> +
>  /* set the default USB model to none for s390 unless an address is found 
> */
>  if (dev->type == VIR_DOMAIN_DEVICE_CONTROLLER &&
>  dev->data.controller->type == VIR_DOMAIN_CONTROLLER_TYPE_USB &&
> 

This would also affect XML parsing, as these PostParse functions are called
when parsing the XML too, not just when doing a XML->native translation.

This also affect all the disks specified on the command line. You shouldn't
assume the disk is SCSI when IDE was explicitly specified.

Jan



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

[libvirt] [PATCH 5/6] virDomainEventCallbackListFree: Don't leak @list->callbacks

2013-11-14 Thread Michal Privoznik

The @list->callbacks is an array that is inflated whenever a new event
is added, e.g. via virDomainEventCallbackListAddID(). However, when we
are freeing the array, we free the items within it but forgot to
actually free it.

Signed-off-by: Michal Privoznik 
---
 src/conf/domain_event.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c
index 16ae92b..19e3920 100644
--- a/src/conf/domain_event.c
+++ b/src/conf/domain_event.c
@@ -147,6 +147,7 @@ 
virDomainEventCallbackListFree(virDomainEventCallbackListPtr list)
 (*freecb)(list->callbacks[i]->opaque);
 VIR_FREE(list->callbacks[i]);
 }
+VIR_FREE(list->callbacks);
 VIR_FREE(list);
 }
 
-- 
1.8.3.2

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

[libvirt] [PATCH 4/6] virThreadPoolFree: Join worker threads

2013-11-14 Thread Michal Privoznik

Even though currently we are freeing the pool of worker threads at the
daemon very end, nothing holds us back in joining the worker threads.

Signed-off-by: Michal Privoznik 
---
 src/util/virthreadpool.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/src/util/virthreadpool.c b/src/util/virthreadpool.c
index b1e2c0c..99f36ec 100644
--- a/src/util/virthreadpool.c
+++ b/src/util/virthreadpool.c
@@ -241,6 +241,9 @@ void virThreadPoolFree(virThreadPoolPtr pool)
 {
 virThreadPoolJobPtr job;
 bool priority = false;
+size_t i;
+size_t nWorkers = pool->nWorkers;
+size_t nPrioWorkers = pool->nPrioWorkers;
 
 if (!pool)
 return;
@@ -262,6 +265,12 @@ void virThreadPoolFree(virThreadPoolPtr pool)
 VIR_FREE(job);
 }
 
+for (i = 0; i < nWorkers; i++)
+virThreadJoin(&pool->workers[i]);
+
+for (i = 0; i < nPrioWorkers; i++)
+virThreadJoin(&pool->prioWorkers[i]);
+
 VIR_FREE(pool->workers);
 virMutexUnlock(&pool->mutex);
 virMutexDestroy(&pool->mutex);
-- 
1.8.3.2

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

[libvirt] [PATCH 6/6] qemuProcessReconnectHelper: Don't create joinable thread

2013-11-14 Thread Michal Privoznik

In the qemuProcessReconnectHelper() a new thread that do all the
interesting work is spawned. The rationale is to not block the daemon
startup process in case of unresponsive qemu. However, the thread
handler is a local variable which gets lost once the control goes out of
scope. Hence, the thread gets leaked. We can avoid this if the thread
isn't make joinable.

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

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index e34f542..f698d47 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3281,7 +3281,7 @@ qemuProcessReconnectHelper(virDomainObjPtr obj,
  */
 virConnectRef(data->conn);
 
-if (virThreadCreate(&thread, true, qemuProcessReconnect, data) < 0) {
+if (virThreadCreate(&thread, false, qemuProcessReconnect, data) < 0) {
 
 virConnectClose(data->conn);
 
-- 
1.8.3.2

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

[libvirt] [PATCH 1/6] networkBuildDhcpDaemonCommandLine: Don't leak @configstr

2013-11-14 Thread Michal Privoznik

In the function an helper to build the dnsmasq config file is used. The
content to be stored then into the config file is kept in @configstr
variable. However, once written, it's never freed and as soon as the
control reaches the 'return' line, the variable (and hence allocated
memory handle) will go off the scope and is leaked.

Signed-off-by: Michal Privoznik 
---
 src/network/bridge_driver.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 3423a45..fd4dfc9 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -1061,6 +1061,7 @@ networkBuildDhcpDaemonCommandLine(virNetworkObjPtr 
network,
 *cmdout = cmd;
 ret = 0;
 cleanup:
+VIR_FREE(configstr);
 return ret;
 }
 
-- 
1.8.3.2

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

[libvirt] [PATCH 2/6] networkBuildDhcpDaemonCommandLine: Don't leak @configfile

2013-11-14 Thread Michal Privoznik

The name of the dnsmasq's config file is initialized and passed to
virCommand. However, the virCommand creates a copy, so we are
responsible for freeing the @configfile ourselves.

Signed-off-by: Michal Privoznik 
---
 src/network/bridge_driver.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index fd4dfc9..4298576 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -1061,6 +1061,7 @@ networkBuildDhcpDaemonCommandLine(virNetworkObjPtr 
network,
 *cmdout = cmd;
 ret = 0;
 cleanup:
+VIR_FREE(configfile);
 VIR_FREE(configstr);
 return ret;
 }
-- 
1.8.3.2

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

[libvirt] [PATCH 0/6] Fix couple of memleaks

2013-11-14 Thread Michal Privoznik

Couple of patches I had lying around. I expect 4/6 to be controversial a bit.
But it shouldn't be a show stopper for others.

Michal Privoznik (6):
  networkBuildDhcpDaemonCommandLine: Don't leak @configstr
  networkBuildDhcpDaemonCommandLine: Don't leak @configfile
  virPCIDeviceBindToStub: Remove unused @oldDriverPath and
@oldDriverName
  virThreadPoolFree: Join worker threads
  virDomainEventCallbackListFree: Don't leak @list->callbacks
  qemuProcessReconnectHelper: Don't create joinable thread

 src/conf/domain_event.c | 1 +
 src/network/bridge_driver.c | 2 ++
 src/qemu/qemu_process.c | 2 +-
 src/util/virpci.c   | 4 
 src/util/virthreadpool.c| 9 +
 5 files changed, 13 insertions(+), 5 deletions(-)

-- 
1.8.3.2

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

[libvirt] [PATCH 3/6] virPCIDeviceBindToStub: Remove unused @oldDriverPath and @oldDriverName

2013-11-14 Thread Michal Privoznik

These two chunks had to be part of df4283a55bf. But for some unclear
reason, the weren't. Anyway, these two variables are not used anywhere
within function. They're initialized to NULL and then VIR_FREE()-d. And
there's no reason do do two NOPs, right?

Signed-off-by: Michal Privoznik 
---
 src/util/virpci.c | 4 
 1 file changed, 4 deletions(-)

diff --git a/src/util/virpci.c b/src/util/virpci.c
index a41a22b..8a3a492 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -1085,8 +1085,6 @@ virPCIDeviceBindToStub(virPCIDevicePtr dev,
 
 char *stubDriverPath = NULL;
 char *driverLink = NULL;
-char *oldDriverPath = NULL;
-char *oldDriverName = NULL;
 char *path = NULL; /* reused for different purposes */
 const char *newDriverName = NULL;
 
@@ -1217,8 +1215,6 @@ remove_id:
 cleanup:
 VIR_FREE(stubDriverPath);
 VIR_FREE(driverLink);
-VIR_FREE(oldDriverPath);
-VIR_FREE(oldDriverName);
 VIR_FREE(path);
 
 if (newDriverName &&
-- 
1.8.3.2

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

Re: [libvirt] [PATCH v2]lxc: don't mount dir if ownership couldn't be known

2013-11-14 Thread Chen Hanxiao



> -Original Message-
> From: Daniel P. Berrange [mailto:berra...@redhat.com]
> Sent: Wednesday, November 13, 2013 6:35 PM
> To: Chen Hanxiao
> Cc: libvir-list@redhat.com
> Subject: Re: [libvirt] [PATCH v2]lxc: don't mount dir if ownership couldn't be
> known
> 
> On Wed, Nov 13, 2013 at 04:51:43PM +0800, Chen Hanxiao wrote:
> > From: Chen Hanxiao 
> >
> > If we enable userns, we could bind mount
> > some dirs from host to guest, which don't belong to
> > the target mapped uid/gid.
> >
> > Such as we could bind mount root's dirs to guest.
> > What is worse, we could even modify root's files
> > in that bind dir inside container.
> 
> I still can't see what the problem is from the description
> here. Please can you give a clear example of the config
> used and exactly what goes wrong.
> 

1. enable user namespace
  


  

2. bind mount some dirs to container, which belongs to root or other users.

  
  


# ll /media/
...
drwxr-xr-x.  3 root root4096 Nov 13 17:21 LXC1
...

3. start container

I used to encounter issues: inside container, we could modify files under /mnt

So I think inside user namespace, if we do not have a proper id mapping,
we should not bind mount it for containers, or at least set it as readonly.




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

Re: [libvirt] [PATCH] LXC: make sure fuse thread start to run before we do clone

2013-11-14 Thread Daniel P. Berrange

On Wed, Nov 13, 2013 at 11:33:46AM -0700, Eric Blake wrote:
> On 11/13/2013 11:16 AM, Rich Felker wrote:
> 
> >> We are not using clone() in a manner that is strictly equivalent
> >> to fork(). Libvirt is using clone() to create Linux containers
> >> with new namespaces. eg we do 
> >>
> >>   
> >> clone(CLONE_NEWPID|CLONE_NEWNS|CLONE_NEWUTS|CLONE_NEWIPC|CLONE_NEWUSER|CLONE_NEWNET|SIGCHLD)
> > 
> > Understood. I still call this a fork-like manner since it's not
> > sharing VM or using CLONE_THREAD and using the default signal of
> > SIGCHLD. BTW is there a reason to prefer this usage over regular fork
> > followed by unshare()?
> 
> Yes.  Per 'man 2 unshare', CLONE_NEWPID is not supported with unshare(),
> yet we require our child to have pid 1 in its new pid namespace.

Yeah, I also wish we could use unshare() instead of clone(), but the
CLONE_NEWPID design limitation is a blocker for that.

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

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

Re: [libvirt] [PATCH v2] virsh domxml-from-native to treat SCSI as the bus type for pseries by default

2013-11-14 Thread Shivaprasad bhat

Thanks Cole.

The change is to correct the IDE disk type to SCSI on pseries systems for
domxml-from-native.

Here is the test case and results.

sh# cat cmd.txt
*qemu-system-ppc64 -M pseries -m 4096 -nographic -enable-kvm -hda
/data/images/rhel70.qcow2 -name rhel70 -cdrom
/data/iso/RHEL-7.0-20130306.0-Server-ppc64-dvd1.iso -boot d -vnc :30*

sh# ./run tools/*virsh domxml-from-native qemu-argv cmd.txt*

  rhel70
  eae019ae-a155-4dd8-be21-f9738b6aedea
  4194304
  4194304
  1
  
hvm

  
  
  destroy
  restart
  destroy
  
qemu-system-ppc64

  
  
*  //> ide is
changed to SCSI*
  


  
  
*   //> ide is
changed to SCSI*
  
  

  *   //> Controller ide
also changed to SCSI*




  


  


  

sh#

Regards,
Shiva



On Thu, Nov 14, 2013 at 3:02 AM, Cole Robinson  wrote:

> On 11/13/2013 04:31 PM, Shivaprasad bhat wrote:
> > Hi Jan, Cole,
> >
> > Could you please reviewing my patch ?
> >
> > Thanks,
> > Shiva
> >
>
> I'd recommend adding a test case that demonstrates what exactly this is
> changing.
>
> - Cole
>
> >
> > On Wed, Oct 30, 2013 at 1:37 PM, Shivaprasad bhat <
> shivaprasadb...@gmail.com
> > > wrote:
> >
> > Hi,
> >
> > Could someone please help reviewing the patch ?
> >
> > Thanks and Regards,
> > Shiva
> >
> >
> > On Mon, Oct 28, 2013 at 2:50 PM, Shivaprasad G Bhat
> > mailto:shivaprasadb...@gmail.com>>
> wrote:
> >
> > The bus type IDE being enum Zero, the bus type on pseries system
> > appears as IDE for all the disk types. Pseries platform needs
> this to
> > appear as SCSI instead of IDE.
> >
> > Signed-off-by: Shivaprasad G Bhat  > >
> > ---
> >  src/qemu/qemu_domain.c |   11 +++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > index b8aec2d..df06c13 100644
> > --- a/src/qemu/qemu_domain.c
> > +++ b/src/qemu/qemu_domain.c
> > @@ -827,6 +827,12 @@
> > qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
> >  if (dev->type == VIR_DOMAIN_DEVICE_DISK) {
> >  virDomainDiskDefPtr disk = dev->data.disk;
> >
> > +if ((def->os.arch == VIR_ARCH_PPC64) &&
> > +def->os.machine && STREQ(def->os.machine,
> "pseries") &&
> > +(disk->bus == VIR_DOMAIN_DISK_BUS_IDE)) {
> > +disk->bus = VIR_DOMAIN_DISK_BUS_SCSI;
> > +}
> > +
> >  /* both of these require data from the driver config */
> >  if (driver && (cfg = virQEMUDriverGetConfig(driver))) {
> >  /* assign default storage format and driver
> according to
> > config */
> > @@ -868,6 +874,11 @@
> > qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
> >  (def->os.arch == VIR_ARCH_S390 || def->os.arch ==
> > VIR_ARCH_S390X))
> >  dev->data.chr->targetType =
> > VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO;
> >
> > +if (dev->type == VIR_DOMAIN_DEVICE_CONTROLLER &&
> > +dev->data.controller->type ==
> VIR_DOMAIN_CONTROLLER_TYPE_IDE &&
> > +def->os.machine && STREQ(def->os.machine, "pseries"))
> > +dev->data.controller->type =
> VIR_DOMAIN_CONTROLLER_TYPE_SCSI;
> > +
> >  /* set the default USB model to none for s390 unless an
> address
> > is found */
> >  if (dev->type == VIR_DOMAIN_DEVICE_CONTROLLER &&
> >  dev->data.controller->type ==
> VIR_DOMAIN_CONTROLLER_TYPE_USB &&
> >
> > --
> > libvir-list mailing list
> > libvir-list@redhat.com 
> > https://www.redhat.com/mailman/listinfo/libvir-list
> >
> >
> >
>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

44 matches

Mail list logo