3:59 p.m.
Jim suggested that it might be useful for me to make a repository available
with my work-in-progress storage patches even if they're not ready to merge
just yet.
I work using a mercurial patch queue, which is basically a mercurial native
version of quilt.
The master HG repository is an automated sync of CVS -> HG:
http://hg.berrange.com/libraries/libvirt--devel
The patch queue which applies ontop of this repo is:
http://hg.berrange.com/libraries/libvirt--storage
To use this you would try the following
1. Make sure the 'mg' extension is enabled in $HOME/.hgrc. You should have:
$ cat $HOME/.hgrc
[extensions]
hgext.mq=
2. Clone both repos in one go:
hg qclone -p http://hg.berrange.com/libraries/libvirt--storage \
http://hg.berrange.com/libraries/libvirt--devel
3. The local 'libvirt-storage' directory now contains the master repo,
along with an embedded patch queue. You can view the patch queue:
$ hg qseries
ignore-rules
misc-connection-hash
misc-libvirt-debug
misc-virsh-read-all
misc-default-uri
job-api
job-virsh
job-test-thread-safety
job-test-driver
storage-public-api
storage-driver-api
storage-remote-protocol
storage-remote-driver
storage-virsh
storage-virsh-args
storage-driver
storage-examples
storage-backend-helpers
storage-backend-fs
storage-backend-loop
storage-backend-lvm
storage-backend-iscsi
storage-backend-disk
storage-python-api
misc-python-generator
4. And you can apply / remove patches to/from the working directory with
the 'hg qpush' and 'hg qpop' commands. 'hg qdiff' will show
contents
of the patches
$ hg qpush
applying ignore-rules
Now at: ignore-rules
$ hg qpush
applying misc-connection-hash
Now at: misc-connection-hash
....etc
When I've tidying up a few loose ends in the next few days I'll post the
current series with proper descriptions of each patch
Dan.
--
|=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=|
|=- Perl modules: http://search.cpan.org/~danberr/ -=|
|=- Projects: http://freshmeat.net/~danielpb/ -=|
|=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
2:32 a.m.
On Sat, 2008-01-12 at 21:59 +0000, Daniel P. Berrange wrote:
I work using a mercurial patch queue, which is basically a mercurial
native
version of quilt.
The master HG repository is an automated sync of CVS -> HG:
http://hg.berrange.com/libraries/libvirt--devel
The patch queue which applies ontop of this repo is:
http://hg.berrange.com/libraries/libvirt--storage
To use this you would try the following
1. Make sure the 'mg' extension is enabled in $HOME/.hgrc. You should have:
People could just use this patch queue directly on top of a CVS
checkout using quilt if .hgignore was committed to CVS ...
Cheers,
Mark.
2:39 a.m.
Mark McLoughlin <markmc(a)redhat.com> wrote:
On Sat, 2008-01-12 at 21:59 +0000, Daniel P. Berrange wrote:
> I work using a mercurial patch queue, which is basically a mercurial native
> version of quilt.
>
> The master HG repository is an automated sync of CVS -> HG:
>
> http://hg.berrange.com/libraries/libvirt--devel
>
> The patch queue which applies ontop of this repo is:
>
> http://hg.berrange.com/libraries/libvirt--storage
>
>
> To use this you would try the following
>
> 1. Make sure the 'mg' extension is enabled in $HOME/.hgrc. You should have:
People could just use this patch queue directly on top of a CVS
checkout using quilt if .hgignore was committed to CVS ...
Hi Mark,
I'll bet you have one handy.
Is there any objection to committing it, now?
3:50 a.m.
On Mon, 2008-01-14 at 09:39 +0100, Jim Meyering wrote:
Mark McLoughlin <markmc(a)redhat.com> wrote:
> On Sat, 2008-01-12 at 21:59 +0000, Daniel P. Berrange wrote:
>> I work using a mercurial patch queue, which is basically a mercurial native
>> version of quilt.
>>
>> The master HG repository is an automated sync of CVS -> HG:
>>
>> http://hg.berrange.com/libraries/libvirt--devel
>>
>> The patch queue which applies ontop of this repo is:
>>
>> http://hg.berrange.com/libraries/libvirt--storage
>>
>>
>> To use this you would try the following
>>
>> 1. Make sure the 'mg' extension is enabled in $HOME/.hgrc. You should
have:
>
> People could just use this patch queue directly on top of a CVS
> checkout using quilt if .hgignore was committed to CVS ...
Hi Mark,
I'll bet you have one handy.
Yep :-)
http://hg.berrange.com/libraries/libvirt--devel?f=7dba49fef963;file=.hgig...
My point was only that Dan's mq patches assume the existance
of .hgignore, which isn't in CVS, so you can't use the patch queue with
quilt and CVS.
Cheers,
Mark.
2:38 a.m.
"Daniel P. Berrange" <berrange(a)redhat.com> wrote:
2. Clone both repos in one go:
hg qclone -p http://hg.berrange.com/libraries/libvirt--storage \
http://hg.berrange.com/libraries/libvirt--devel
Hi Dan,
Thanks for setting that up.
9:16 a.m.
On Sat, Jan 12, 2008 at 09:59:22PM +0000, Daniel P. Berrange wrote:
Jim suggested that it might be useful for me to make a repository
available
with my work-in-progress storage patches even if they're not ready to merge
just yet.
I work using a mercurial patch queue, which is basically a mercurial native
version of quilt.
The master HG repository is an automated sync of CVS -> HG:
http://hg.berrange.com/libraries/libvirt--devel
The patch queue which applies ontop of this repo is:
http://hg.berrange.com/libraries/libvirt--storage
To use this you would try the following
1. Make sure the 'mg' extension is enabled in $HOME/.hgrc. You should have:
$ cat $HOME/.hgrc
[extensions]
hgext.mq=
2. Clone both repos in one go:
hg qclone -p http://hg.berrange.com/libraries/libvirt--storage \
http://hg.berrange.com/libraries/libvirt--devel
3. The local 'libvirt-storage' directory now contains the master repo,
along with an embedded patch queue. You can view the patch queue:
$ hg qseries
ignore-rules
misc-connection-hash
misc-libvirt-debug
misc-virsh-read-all
misc-default-uri
job-api
job-virsh
job-test-thread-safety
job-test-driver
storage-public-api
storage-driver-api
storage-remote-protocol
storage-remote-driver
storage-virsh
storage-virsh-args
storage-driver
storage-examples
storage-backend-helpers
storage-backend-fs
storage-backend-loop
storage-backend-lvm
storage-backend-iscsi
storage-backend-disk
storage-python-api
misc-python-generator
BTW, this last patch does not apply or compile & is just some experiments I
was doing with the python generator to see if I could make it more useful
for 'hard' functions.
Dan.
--
|=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=|
|=- Perl modules: http://search.cpan.org/~danberr/ -=|
|=- Projects: http://freshmeat.net/~danielpb/ -=|
|=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
1:44 p.m.
A few comments:
You should put #include <config.h> at the very top of all your C files
(before any other headers/defines), to avoid warnings.
There was one place where you'd used virFreeStoragePool, which should be
virStoragePoolFree.
Functions such as 'virDomainDestroy' have changed their semantics in
that they now free the virDomainPtr object. I understand why you do
this (the domain no longer exists, so the object cannot be used), but it
is a big change from the p.o.v. of those of us using proper garbage
collection, and moreover it's an ABI change. Existing correct code
which did 'virDomainDestroy (dom); virDomainFree (dom);' could now segfault.
Rich.
--
Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/
Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod
Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in
England and Wales under Company Registration No. 03798903
1:55 p.m.
On Fri, Jan 18, 2008 at 07:44:41PM +0000, Richard W.M. Jones wrote:
A few comments:
You should put #include <config.h> at the very top of all your C files
(before any other headers/defines), to avoid warnings.
There was one place where you'd used virFreeStoragePool, which should be
virStoragePoolFree.
Functions such as 'virDomainDestroy' have changed their semantics in
that they now free the virDomainPtr object. I understand why you do
this (the domain no longer exists, so the object cannot be used), but it
is a big change from the p.o.v. of those of us using proper garbage
collection, and moreover it's an ABI change. Existing correct code
which did 'virDomainDestroy (dom); virDomainFree (dom);' could now segfault.
No it isn't an ABI change. The virDomainDestroy method has always
behaved this way. I simply moved the calls to free out of the per
driver destroy method, and into the top level libvirt.c destroy
method to remove the need for drivers to re-implement this every
time.
Dan
--
|=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=|
|=- Perl modules: http://search.cpan.org/~danberr/ -=|
|=- Projects: http://freshmeat.net/~danielpb/ -=|
|=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
2:04 p.m.
On Fri, Jan 18, 2008 at 07:55:35PM +0000, Daniel P. Berrange wrote:
On Fri, Jan 18, 2008 at 07:44:41PM +0000, Richard W.M. Jones wrote:
> A few comments:
>
> You should put #include <config.h> at the very top of all your C files
> (before any other headers/defines), to avoid warnings.
>
> There was one place where you'd used virFreeStoragePool, which should be
> virStoragePoolFree.
>
> Functions such as 'virDomainDestroy' have changed their semantics in
> that they now free the virDomainPtr object. I understand why you do
> this (the domain no longer exists, so the object cannot be used), but it
> is a big change from the p.o.v. of those of us using proper garbage
> collection, and moreover it's an ABI change. Existing correct code
> which did 'virDomainDestroy (dom); virDomainFree (dom);' could now
segfault.
No it isn't an ABI change. The virDomainDestroy method has always
behaved this way. I simply moved the calls to free out of the per
driver destroy method, and into the top level libvirt.c destroy
method to remove the need for drivers to re-implement this every
time.
Actually, no I take that back. The qemu driver always did this. The Xen
driver did not. So the QEMU driver is actually buggy. I'll remove this
free'ing of virDomainPtr from virDomainDestroy and fix the QEMU driver.
Dan.
--
|=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=|
|=- Perl modules: http://search.cpan.org/~danberr/ -=|
|=- Projects: http://freshmeat.net/~danielpb/ -=|
|=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
8:37 a.m.
Daniel P. Berrange wrote:
On Fri, Jan 18, 2008 at 07:55:35PM +0000, Daniel P. Berrange wrote:
> On Fri, Jan 18, 2008 at 07:44:41PM +0000, Richard W.M. Jones wrote:
>> A few comments:
>>
>> You should put #include <config.h> at the very top of all your C files
>> (before any other headers/defines), to avoid warnings.
>>
>> There was one place where you'd used virFreeStoragePool, which should be
>> virStoragePoolFree.
>>
>> Functions such as 'virDomainDestroy' have changed their semantics in
>> that they now free the virDomainPtr object. I understand why you do
>> this (the domain no longer exists, so the object cannot be used), but it
>> is a big change from the p.o.v. of those of us using proper garbage
>> collection, and moreover it's an ABI change. Existing correct code
>> which did 'virDomainDestroy (dom); virDomainFree (dom);' could now
segfault.
> No it isn't an ABI change. The virDomainDestroy method has always
> behaved this way. I simply moved the calls to free out of the per
> driver destroy method, and into the top level libvirt.c destroy
> method to remove the need for drivers to re-implement this every
> time.
Actually, no I take that back. The qemu driver always did this. The Xen
driver did not. So the QEMU driver is actually buggy. I'll remove this
free'ing of virDomainPtr from virDomainDestroy and fix the QEMU driver.
Actually I think I'm getting confused now.
The current (in CVS) documentation for virDomainDestroy &
virNetworkDestroy documents that they free the object. So bug in the
Xen driver instead?
Rich.
--
Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/
Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod
Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in
England and Wales under Company Registration No. 03798903
12:01 p.m.
On Sat, Jan 19, 2008 at 02:37:51PM +0000, Richard W.M. Jones wrote:
Daniel P. Berrange wrote:
>On Fri, Jan 18, 2008 at 07:55:35PM +0000, Daniel P. Berrange wrote:
>>On Fri, Jan 18, 2008 at 07:44:41PM +0000, Richard W.M. Jones wrote:
>>>A few comments:
>>>
>>>You should put #include <config.h> at the very top of all your C files
>>>(before any other headers/defines), to avoid warnings.
>>>
>>>There was one place where you'd used virFreeStoragePool, which should be
>>>virStoragePoolFree.
>>>
>>>Functions such as 'virDomainDestroy' have changed their semantics in
>>>that they now free the virDomainPtr object. I understand why you do
>>>this (the domain no longer exists, so the object cannot be used), but it
>>>is a big change from the p.o.v. of those of us using proper garbage
>>>collection, and moreover it's an ABI change. Existing correct code
>>>which did 'virDomainDestroy (dom); virDomainFree (dom);' could now
>>>segfault.
>>No it isn't an ABI change. The virDomainDestroy method has always
>>behaved this way. I simply moved the calls to free out of the per
>>driver destroy method, and into the top level libvirt.c destroy
>>method to remove the need for drivers to re-implement this every
>>time.
>
>Actually, no I take that back. The qemu driver always did this. The Xen
>driver did not. So the QEMU driver is actually buggy. I'll remove this
>free'ing of virDomainPtr from virDomainDestroy and fix the QEMU driver.
Actually I think I'm getting confused now.
The current (in CVS) documentation for virDomainDestroy &
virNetworkDestroy documents that they free the object. So bug in the
Xen driver instead?
Well, personally I'll take actual application visible behaviour over
docs. The behaviour of the QEMU driver in free'ing the object is not
exposed to any apps, since QEMU driver is only acessed by the daemon,
and apps get to it indirectly via the remote driver. So if neither
the remote driver, nor Xen driver free the objects that is a defacto
API contract regardless of docs.
Regards,
Dan.
--
|=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=|
|=- Perl modules: http://search.cpan.org/~danberr/ -=|
|=- Projects: http://freshmeat.net/~danielpb/ -=|
|=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
7:19 a.m.
Another thing is that there's confusion over where 'flags' parameters
should be unsigned or not. The confusion comes from existing calls
which are a bit of a hodge-podge, even using 'long' in one case:
virConnectOpenAuth int flags
virDomainCreateLinux unsigned int flags
virDomainCreateLinuxJob unsigned int flags
virDomainReboot unsigned int flags
virDomainCoreDump int flags
virDomainCoreDumpJob int flags
virDomainGetXMLDesc int flags
virDomainCreateJob int flags
virDomainMigrate unsigned long flags
virNetworkGetXMLDesc int flags
virConnectDiscoverStoragePools unsigned int flags
virStoragePoolRefresh unsigned int flags
virStoragePoolGetXMLDesc int flags
virStorageVolCreateXML int flags
virStorageVolGetXMLDesc int flags
I propose that we change all except virDomainMigrate to 'unsigned int'.
That shouldn't break ABI (right?)
Rich.
--
Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/
Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod
Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in
England and Wales under Company Registration No. 03798903
11:58 a.m.
On Sat, Jan 19, 2008 at 01:19:35PM +0000, Richard W.M. Jones wrote:
Another thing is that there's confusion over where
'flags' parameters
should be unsigned or not. The confusion comes from existing calls
which are a bit of a hodge-podge, even using 'long' in one case:
virConnectOpenAuth int flags
virDomainCreateLinux unsigned int flags
virDomainCreateLinuxJob unsigned int flags
virDomainReboot unsigned int flags
virDomainCoreDump int flags
virDomainCoreDumpJob int flags
virDomainGetXMLDesc int flags
virDomainCreateJob int flags
virDomainMigrate unsigned long flags
virNetworkGetXMLDesc int flags
virConnectDiscoverStoragePools unsigned int flags
virStoragePoolRefresh unsigned int flags
virStoragePoolGetXMLDesc int flags
virStorageVolCreateXML int flags
virStorageVolGetXMLDesc int flags
I propose that we change all except virDomainMigrate to 'unsigned int'.
That shouldn't break ABI (right?)
Yeah, I don't believe it would break anything - easy enough to check by
changing it and seeing if all our various apps still compile OK.
Dan.
--
|=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=|
|=- Perl modules: http://search.cpan.org/~danberr/ -=|
|=- Projects: http://freshmeat.net/~danielpb/ -=|
|=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
7:47 a.m.
This function confuses me a bit. It takes a virStoragePoolPtr as
parameter, but it only uses pool->conn. The other two
virStorageVolLookupBy* functions take a virConnectPtr directly.
virStorageVolPtr
virStorageVolLookupByName(virStoragePoolPtr pool,
const char *name)
{
DEBUG("pool=%p, name=%s", pool, name);
if (!VIR_IS_STORAGE_POOL(pool)) {
virLibConnError(NULL, VIR_ERR_INVALID_CONN, __FUNCTION__);
return (NULL);
}
if (name == NULL) {
virLibConnError(pool->conn, VIR_ERR_INVALID_ARG, __FUNCTION__);
return (NULL);
}
if (pool->conn->storageDriver &&
pool->conn->storageDriver->volLookupByName)
return pool->conn->storageDriver->volLookupByName (pool, name);
/* ^^ is a red herring, since it's not implemented in
anything except the remote driver, hence useless */
virLibConnError (pool->conn, VIR_ERR_NO_SUPPORT, __FUNCTION__);
return NULL;
}
Rich.
--
Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/
Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod
Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in
England and Wales under Company Registration No. 03798903
11:57 a.m.
On Sat, Jan 19, 2008 at 01:47:30PM +0000, Richard W.M. Jones wrote:
This function confuses me a bit. It takes a virStoragePoolPtr as
parameter, but it only uses pool->conn. The other two
virStorageVolLookupBy* functions take a virConnectPtr directly.
There are 3 levels of unique identifiers in storage volumes
- name - unique within the scope of a Pool
- key - unique across any machine accessing the same pool
- path - unique within scope of a host (optionally across any host,
if the pool impl supports that).
So, since name is unique within scope of a volume, while the others
are unique within scope of a host, the virStorageVolLookupByName
method is different, taking a virStoragePoolPtr instead of a
virConnectPtr.
return pool->conn->storageDriver->volLookupByName
(pool, name);
/* ^^ is a red herring, since it's not implemented in
anything except the remote driver, hence useless */
It is also in the storage_driver.c - storageVolumeLookupByName
Regards,
Dan.
--
|=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=|
|=- Perl modules: http://search.cpan.org/~danberr/ -=|
|=- Projects: http://freshmeat.net/~danielpb/ -=|
|=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
6:20 a.m.
Daniel P. Berrange wrote:
On Sat, Jan 19, 2008 at 01:47:30PM +0000, Richard W.M. Jones wrote:
> This function confuses me a bit. It takes a virStoragePoolPtr as
> parameter, but it only uses pool->conn. The other two
> virStorageVolLookupBy* functions take a virConnectPtr directly.
There are 3 levels of unique identifiers in storage volumes
- name - unique within the scope of a Pool
- key - unique across any machine accessing the same pool
- path - unique within scope of a host (optionally across any host,
if the pool impl supports that).
So, since name is unique within scope of a volume, while the others
are unique within scope of a host, the virStorageVolLookupByName
method is different, taking a virStoragePoolPtr instead of a
virConnectPtr.
A few examples would go a long way to helping me understand this. Can
you give examples of name/key/path in the context of
iscsi/partition/directory?
Rich.
--
Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/
Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod
Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in
England and Wales under Company Registration No. 03798903
9:13 a.m.
On Sun, Jan 20, 2008 at 12:20:03PM +0000, Richard W.M. Jones wrote:
Daniel P. Berrange wrote:
>On Sat, Jan 19, 2008 at 01:47:30PM +0000, Richard W.M. Jones wrote:
>>This function confuses me a bit. It takes a virStoragePoolPtr as
>>parameter, but it only uses pool->conn. The other two
>>virStorageVolLookupBy* functions take a virConnectPtr directly.
>
>There are 3 levels of unique identifiers in storage volumes
>
> - name - unique within the scope of a Pool
> - key - unique across any machine accessing the same pool
> - path - unique within scope of a host (optionally across any host,
> if the pool impl supports that).
>
>So, since name is unique within scope of a volume, while the others
>are unique within scope of a host, the virStorageVolLookupByName
>method is different, taking a virStoragePoolPtr instead of a
>virConnectPtr.
A few examples would go a long way to helping me understand this. Can
you give examples of name/key/path in the context of
iscsi/partition/directory?
The key stuff is not really implemented yet, but the examples of how
it would look are:
* iscsi: There are a choice of paths - if no /dev/disk/by-* is specified
then it falls back to non-stable /dev/sd*
name: '0:2:0:1'
path: /dev/disk/by-path/ip-192.168.122.170:3260-iscsi-demo-tgt-a-lun-3
/dev/disk/by-id/.... (can't remember format of this offhand)
/dev/sdc
key: ip-192.168.122.170:3260-iscsi-demo-tgt-a-lun-3
* disk: Again a choice of paths
name: part1
path: /dev/sda1
/dev/disk/by-path/pci-0000:00:1f.2-scsi-0:0:0:0-part1
/dev/disk/by-id/scsi-SATA_HTS721010G9SA00_MPCZ12Y0GNGWSE-part1
key: TBD (will extract unique key from sysfs metadata)
* directory:
name: demo.img
path: /var/lib/xen/images/demo.img
key: combo of unique key of underlying block dev + inode
The directory part of the 'path' in all these examples is determined
by the value of the <target>/some/dir/</target> parameter in the pool
definition.
For block devices we really need to stay away from /dev/sd* nodes
whenever possible since they are effectively randomly allocated at
boot time, no not really stable. So when defining a pool that uses
block devices the recommendation will be to specify a <target>
element pointing to /dev/disk/by-id or /dev/disk/by-uuid which is
where predictable stable paths live. These stable paths are created
by udev rules. It is apparently fairly common for people to create
their own additional udev rules to provide custom naming schemes,
so allowing it to be specified with <target></target> in the pool is
quite handy.
Btw, patch 'storage-examples' sticks some example XML inputs into the
docs/storage directory.
Regards,
Dan.
--
|=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=|
|=- Perl modules: http://search.cpan.org/~danberr/ -=|
|=- Projects: http://freshmeat.net/~danielpb/ -=|
|=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
3:01 p.m.
On Fri, Jan 18, 2008 at 07:44:41PM +0000, Richard W.M. Jones wrote:
A few comments:
You should put #include <config.h> at the very top of all your C files
(before any other headers/defines), to avoid warnings.
I believe I've added that to all files I created now.
There was one place where you'd used virFreeStoragePool, which
should be
virStoragePoolFree.
Fixed that too.
I've push a new series of patches which fix these, and incorporate feedback
and fixes from Jim too. Should apply to current libvirt CVS.
Dan.
--
|=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=|
|=- Perl modules: http://search.cpan.org/~danberr/ -=|
|=- Projects: http://freshmeat.net/~danielpb/ -=|
|=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
12:09 p.m.
"Daniel P. Berrange" <berrange(a)redhat.com> wrote:
...
Since you're working on the weekend ;-), here are some
notices I'd begun to accumulate:
There are a bunch of new uses of open64, which isn't portable.
How about using AC_SYS_LARGEFILE in configure.in instead?
Then you can use "open" everywhere.
In storage_backend_loop.c, it looks like vol->target.path can be leaked.
Just after that, I wondered if other vol->members could be leaked,
but haven't yet looked at the cleanup-called vol-destroying
function; it probably frees everything.
12:13 p.m.
On Sat, Jan 19, 2008 at 07:09:31PM +0100, Jim Meyering wrote:
"Daniel P. Berrange" <berrange(a)redhat.com> wrote:
...
Since you're working on the weekend ;-), here are some
notices I'd begun to accumulate:
There are a bunch of new uses of open64, which isn't portable.
How about using AC_SYS_LARGEFILE in configure.in instead?
Then you can use "open" everywhere.
In storage_backend_loop.c, it looks like vol->target.path can be leaked.
Which function is that in ? Since originally writing it I've changed all
error path cleanup code to simply call virStorageVolDefFree(), so there's
a central function which will free all members, rather than having cleanup
duplicated all over the place.
Just after that, I wondered if other vol->members could be
leaked,
but haven't yet looked at the cleanup-called vol-destroying
function; it probably frees everything.
Yep, check virStorageVolDefFree() - that's intended to be the only place
where vol->members are cleaned up.
Dan.
--
|=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=|
|=- Perl modules: http://search.cpan.org/~danberr/ -=|
|=- Projects: http://freshmeat.net/~danielpb/ -=|
|=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
12:42 p.m.
"Daniel P. Berrange" <berrange(a)redhat.com> wrote:
On Sat, Jan 19, 2008 at 07:09:31PM +0100, Jim Meyering wrote:
...
> In storage_backend_loop.c, it looks like vol->target.path
can be leaked.
Which function is that in ? Since originally writing it I've changed all
error path cleanup code to simply call virStorageVolDefFree(), so there's
a central function which will free all members, rather than having cleanup
duplicated all over the place.
This is the state from yesterday:
static int virStorageBackendLoopVolCreate(virConnectPtr conn,
...
if ((vol->target.path = malloc(5 + strlen(vol->name) + 1)) == NULL) {
virStorageReportError(conn, VIR_ERR_NO_MEMORY, "target");
return -1;
}
strcpy(vol->target.path, "/dev/");
strcat(vol->target.path, vol->name);
if ((vol->key = strdup(vol->target.path)) == NULL) {
virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, "storage vol key");
return -1;
}
if (virRun(conn, (char**)cmdargv, NULL) < 0)
return -1;
if ((fd = open64(vol->target.path, O_RDONLY)) < 0) {
virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, "cannot read path
'%s': %d (%s)",
vol->target.path, errno, strerror(errno));
goto cleanup;
}
...
cleanup:
close(fd);
virStorageBackendLoopVolDelete(conn, pool, vol);
return -1;
}
-----------------------------
I'd say just do s/return -1/goto cleanup/ after failed strdup
but then you have to be careful to set fd = -1, and avoid calling
"close" on it in that case.
12:52 p.m.
On Sat, Jan 19, 2008 at 07:42:35PM +0100, Jim Meyering wrote:
"Daniel P. Berrange" <berrange(a)redhat.com> wrote:
> On Sat, Jan 19, 2008 at 07:09:31PM +0100, Jim Meyering wrote:
...
>> In storage_backend_loop.c, it looks like vol->target.path can be leaked.
>
> Which function is that in ? Since originally writing it I've changed all
> error path cleanup code to simply call virStorageVolDefFree(), so there's
> a central function which will free all members, rather than having cleanup
> duplicated all over the place.
This is the state from yesterday:
static int virStorageBackendLoopVolCreate(virConnectPtr conn,
...
if ((vol->target.path = malloc(5 + strlen(vol->name) + 1)) == NULL) {
virStorageReportError(conn, VIR_ERR_NO_MEMORY, "target");
return -1;
}
strcpy(vol->target.path, "/dev/");
strcat(vol->target.path, vol->name);
if ((vol->key = strdup(vol->target.path)) == NULL) {
virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, "storage vol
key");
return -1;
}
if (virRun(conn, (char**)cmdargv, NULL) < 0)
return -1;
if ((fd = open64(vol->target.path, O_RDONLY)) < 0) {
virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, "cannot read path
'%s': %d (%s)",
vol->target.path, errno, strerror(errno));
goto cleanup;
}
...
cleanup:
close(fd);
virStorageBackendLoopVolDelete(conn, pool, vol);
return -1;
}
-----------------------------
I'd say just do s/return -1/goto cleanup/ after failed strdup
but then you have to be careful to set fd = -1, and avoid calling
"close" on it in that case.
Actually that's OK - the contract of the 'volCreate' method in the backend
does not require free'ing of the 'vol' object upon failure. Becaue
'vol'
is passed in pre-allocated, it is the caller's responsibilty to release
the 'vol' object upon failure. So the cleanup code in the loop driver
only needs to cleanup mess it made itself - eg releasing the loop device
and closing the fd.
Take a look at the storageVolumeCreateXML() method in storage_driver.c
+ if (backend->createVol(obj->conn, pool, vol) < 0) {
+ virStorageVolDefFree(vol);
+ return NULL;
+ }
That 'createVol' call is what's invoking virStorageBackendLoopVolCreate()
I should document this API contract alongside the driver API to make
this clear.
Regards,
Dan.
--
|=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=|
|=- Perl modules: http://search.cpan.org/~danberr/ -=|
|=- Projects: http://freshmeat.net/~danielpb/ -=|
|=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
1:05 p.m.
"Daniel P. Berrange" <berrange(a)redhat.com> wrote:
Actually that's OK - the contract of the 'volCreate'
method in the backend
does not require free'ing of the 'vol' object upon failure. Becaue
'vol'
is passed in pre-allocated, it is the caller's responsibilty to release
the 'vol' object upon failure. So the cleanup code in the loop driver
only needs to cleanup mess it made itself - eg releasing the loop device
and closing the fd.
Ah... got it, now.
Thanks.
12:19 p.m.
On Sat, Jan 19, 2008 at 07:09:31PM +0100, Jim Meyering wrote:
"Daniel P. Berrange" <berrange(a)redhat.com> wrote:
...
Since you're working on the weekend ;-), here are some
notices I'd begun to accumulate:
There are a bunch of new uses of open64, which isn't portable.
How about using AC_SYS_LARGEFILE in configure.in instead?
Then you can use "open" everywhere.
AFAIK, there are two ways to do large file suport
- Explicit support - size_t, open, etc all remain the same, and
new size64_t, open64, etc are introduced.
- Implicit support - size_t, open, etc are re-defined to be 64-bit
at all times.
Both are part of POSIX. With the latter, if any size_t is exposed in your
public API, then all applications linking against you must also be compiled
with large file suport because this is an ABI sensitive think. With the
former approach all the large file stuff is only visible inside your code
so is not leaked to applications using the lib.
Since we have size_t in the public API, AFAICT, we have no choice but to
use the explicit size64_t, open64() etc.
Since both approaches are part of POSIX large file standard I don't see
one as any more or less portable than the other. Both are supported on
Solaris. I don't know about cygwin, but that only requires the remote
driver client code at this time.
Dan.
--
|=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=|
|=- Perl modules: http://search.cpan.org/~danberr/ -=|
|=- Projects: http://freshmeat.net/~danielpb/ -=|
|=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
12:58 p.m.
"Daniel P. Berrange" <berrange(a)redhat.com> wrote:
On Sat, Jan 19, 2008 at 07:09:31PM +0100, Jim Meyering wrote:
> "Daniel P. Berrange" <berrange(a)redhat.com> wrote:
> ...
>
> Since you're working on the weekend ;-), here are some
> notices I'd begun to accumulate:
>
> There are a bunch of new uses of open64, which isn't portable.
> How about using AC_SYS_LARGEFILE in configure.in instead?
> Then you can use "open" everywhere.
AFAIK, there are two ways to do large file suport
- Explicit support - size_t, open, etc all remain the same, and
new size64_t, open64, etc are introduced.
- Implicit support - size_t, open, etc are re-defined to be 64-bit
at all times.
Both are part of POSIX. With the latter, if any size_t is exposed in your
public API, then all applications linking against you must also be compiled
with large file suport because this is an ABI sensitive think. With the
former approach all the large file stuff is only visible inside your code
so is not leaked to applications using the lib.
Since we have size_t in the public API, AFAICT, we have no choice but to
use the explicit size64_t, open64() etc.
large-FILE support does not affect the memory-related size_t
and ssize_t types, and open64/size64_t are not specified by POSIX.
Maybe you're thinking of off_t, since that type does change size
depending on whether large-file support is enabled. But off_t is
not used in libvirt at all.
1:25 p.m.
On Sat, Jan 19, 2008 at 07:58:31PM +0100, Jim Meyering wrote:
"Daniel P. Berrange" <berrange(a)redhat.com> wrote:
> On Sat, Jan 19, 2008 at 07:09:31PM +0100, Jim Meyering wrote:
>> "Daniel P. Berrange" <berrange(a)redhat.com> wrote:
>> ...
>>
>> Since you're working on the weekend ;-), here are some
>> notices I'd begun to accumulate:
>>
>> There are a bunch of new uses of open64, which isn't portable.
>> How about using AC_SYS_LARGEFILE in configure.in instead?
>> Then you can use "open" everywhere.
>
> AFAIK, there are two ways to do large file suport
>
> - Explicit support - size_t, open, etc all remain the same, and
> new size64_t, open64, etc are introduced.
>
> - Implicit support - size_t, open, etc are re-defined to be 64-bit
> at all times.
>
> Both are part of POSIX. With the latter, if any size_t is exposed in your
> public API, then all applications linking against you must also be compiled
> with large file suport because this is an ABI sensitive think. With the
> former approach all the large file stuff is only visible inside your code
> so is not leaked to applications using the lib.
>
> Since we have size_t in the public API, AFAICT, we have no choice but to
> use the explicit size64_t, open64() etc.
large-FILE support does not affect the memory-related size_t
and ssize_t types, and open64/size64_t are not specified by POSIX.
Opps, it seems Large File Support is actually an X/Open UNIX standard
rather than POSIX.
Maybe you're thinking of off_t, since that type does change size
depending on whether large-file support is enabled. But off_t is
not used in libvirt at all.
Yes, I was mistaken about size_t - its only the file related types
that change.
So, if we use AC_SYS_LARGEFILE, then all file related APIs - open,
statfs, statvfs, etc, etc become 64-bit across the whole of libvirt
but doesn't leak out into API. So i'm fine with that.
Dan.
--
|=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=|
|=- Perl modules: http://search.cpan.org/~danberr/ -=|
|=- Projects: http://freshmeat.net/~danielpb/ -=|
|=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
6:27 a.m.
Jim Meyering wrote:
"Daniel P. Berrange" <berrange(a)redhat.com> wrote:
...
Since you're working on the weekend ;-), here are some
notices I'd begun to accumulate:
There are a bunch of new uses of open64, which isn't portable.
How about using AC_SYS_LARGEFILE in configure.in instead?
Then you can use "open" everywhere.
Agreed -- I actually got libvirt + storage to almost compile on Mac OS X
yesterday, except I had to remove open64 and stat64 stuff.
Patch for that later .. Unfortunately Mac OS X ships with a version of
xdr & rpcgen dating from around the time of the Trojan War. It doesn't
support hyper / xdr_quad :-)
Rich.
--
Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/
Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod
Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in
England and Wales under Company Registration No. 03798903
6:28 a.m.
Richard W.M. Jones wrote:
and stat64 stuff.
That should be off64_t.
Rich.
--
Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/
Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod
Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in
England and Wales under Company Registration No. 03798903
9:14 a.m.
On Sun, Jan 20, 2008 at 12:27:09PM +0000, Richard W.M. Jones wrote:
Jim Meyering wrote:
>"Daniel P. Berrange" <berrange(a)redhat.com> wrote:
>...
>
>Since you're working on the weekend ;-), here are some
>notices I'd begun to accumulate:
>
> There are a bunch of new uses of open64, which isn't portable.
> How about using AC_SYS_LARGEFILE in configure.in instead?
> Then you can use "open" everywhere.
Agreed -- I actually got libvirt + storage to almost compile on Mac OS X
yesterday, except I had to remove open64 and stat64 stuff.
The latest patches I pushed to the repo, remove all use of the *64
variants and follow Jim's AC_SYS_LARGEFILE suggestion instead.
Regards,
Dan.
--
|=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=|
|=- Perl modules: http://search.cpan.org/~danberr/ -=|
|=- Projects: http://freshmeat.net/~danielpb/ -=|
|=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
10:33 a.m.
"Daniel P. Berrange" <berrange(a)redhat.com> wrote:
Relative to a snapshot of a few minutes ago, here are two patches:
* src/storage_backend_fs.c (virStorageBackendFileSystemVolCreate):
Save a 4KB memset.
* src/storage_backend_iscsi.c (virStorageBackendISCSIStablePath):
Don't leak a directory handle in failure paths.
Signed-off-by: Jim Meyering <meyering(a)redhat.com>
---
src/storage_backend_fs.c | 3 +--
src/storage_backend_iscsi.c | 7 +++++--
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/src/storage_backend_fs.c b/src/storage_backend_fs.c
index 9e75114..ee17087 100644
--- a/src/storage_backend_fs.c
+++ b/src/storage_backend_fs.c
@@ -606,8 +606,7 @@ static int virStorageBackendFileSystemVolCreate(virConnectPtr conn,
/* XXX slooooooooooooooooow. Need to add in progress bars & bg thread somehow
*/
if (vol->allocation) {
unsigned long long remain = vol->allocation;
- char zeros[4096];
- memset(zeros, 0, sizeof(zeros));
+ static const char const zeros[4096];
while (remain) {
int bytes = sizeof(zeros);
if (bytes > remain)
diff --git a/src/storage_backend_iscsi.c b/src/storage_backend_iscsi.c
index 00dd40c..bbc1608 100644
--- a/src/storage_backend_iscsi.c
+++ b/src/storage_backend_iscsi.c
@@ -38,7 +38,7 @@
#include "util.h"
static int virStorageBackendISCSITargetIP(virConnectPtr conn,
- char *hostname,
+ const char *hostname,
char *ipaddr,
size_t ipaddrlen)
{
@@ -184,6 +184,7 @@ static char *virStorageBackendISCSIStablePath(virConnectPtr conn,
stablepath = malloc(strlen(pool->def->target.path) + 1 +
strlen(dent->d_name) + 1);
if (stablepath == NULL) {
virStorageReportError(conn, VIR_ERR_NO_MEMORY, "path");
+ closedir(dh);
return NULL;
}
@@ -191,8 +192,10 @@ static char *virStorageBackendISCSIStablePath(virConnectPtr conn,
strcat(stablepath, "/");
strcat(stablepath, dent->d_name);
- if (virFileLinkPointsTo(stablepath, devpath))
+ if (virFileLinkPointsTo(stablepath, devpath)) {
+ closedir(dh);
return stablepath;
+ }
free(stablepath);
}
--
1.5.4.rc4.1.g1895
10:57 a.m.
On Mon, Jan 21, 2008 at 05:33:43PM +0100, Jim Meyering wrote:
"Daniel P. Berrange" <berrange(a)redhat.com> wrote:
> hg qclone -p http://hg.berrange.com/libraries/libvirt--storage \
> http://hg.berrange.com/libraries/libvirt--devel
Relative to a snapshot of a few minutes ago, here are two patches:
* src/storage_backend_fs.c (virStorageBackendFileSystemVolCreate):
Save a 4KB memset.
* src/storage_backend_iscsi.c (virStorageBackendISCSIStablePath):
Don't leak a directory handle in failure paths.
Thanks Jim - just incorporated those in the latest patches I pushed
to sync up with CVS tip.
Dan.
--
|=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=|
|=- Perl modules: http://search.cpan.org/~danberr/ -=|
|=- Projects: http://freshmeat.net/~danielpb/ -=|
|=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
6154
days inactive
6163
days old
30 comments
4 participants
participants (4)
-
Daniel P. Berrange -
Jim Meyering -
Mark McLoughlin -
Richard W.M. Jones