[Libvir] Repository for work-in-progress storage patches
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 -=|
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.
Mark McLoughlin <markmc@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?
On Mon, 2008-01-14 at 09:39 +0100, Jim Meyering wrote:
Mark McLoughlin <markmc@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=.hgignor... 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.
"Daniel P. Berrange" <berrange@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.
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 -=|
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
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 -=|
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 -=|
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
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 -=|
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
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 -=|
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
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 -=|
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
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 -=|
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 -=|
"Daniel P. Berrange" <berrange@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.
On Sat, Jan 19, 2008 at 07:09:31PM +0100, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@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 -=|
"Daniel P. Berrange" <berrange@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.
On Sat, Jan 19, 2008 at 07:42:35PM +0100, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@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 -=|
"Daniel P. Berrange" <berrange@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.
On Sat, Jan 19, 2008 at 07:09:31PM +0100, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@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 -=|
"Daniel P. Berrange" <berrange@redhat.com> wrote:
On Sat, Jan 19, 2008 at 07:09:31PM +0100, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@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.
On Sat, Jan 19, 2008 at 07:58:31PM +0100, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote:
On Sat, Jan 19, 2008 at 07:09:31PM +0100, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@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 -=|
Jim Meyering wrote:
"Daniel P. Berrange" <berrange@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
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
On Sun, Jan 20, 2008 at 12:27:09PM +0000, Richard W.M. Jones wrote:
Jim Meyering wrote:
"Daniel P. Berrange" <berrange@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 -=|
"Daniel P. Berrange" <berrange@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. Signed-off-by: Jim Meyering <meyering@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
On Mon, Jan 21, 2008 at 05:33:43PM +0100, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@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 -=|
participants (4)
-
Daniel P. Berrange -
Jim Meyering -
Mark McLoughlin -
Richard W.M. Jones