Re: [libvirt] Google Summer of Code 2013 ideas wiki open
On 07.02.2013 16:19, Stefan Hajnoczi wrote: > I have created the Google Summer of Code 2013 wiki page where you can > add project ideas: > > http://wiki.qemu.org/Google_Summer_of_Code_2013 > > Please add project ideas you are willing to mentor. If you have an > idea but cannot mentor this year, feel free to add it but please try > to find a mentor for it. > > If you want to be a mentor, please see > http://wiki.qemu.org/Google_Summer_of_Code_2013#Information_for_mentors. > I've got some ideas, but I just don't know it their size is sufficient. But IIUC, there are 3 levels (beginner, intermediate and advanced), so hopefully the ideas can land in one of them. 1) Virsh auto completion This is something I think will be very useful for nearly all libvirt users. I've proposed the idea a while ago and my sense is we have some consensus how this should work. But I am afraid this is more mechanical work than real programming one. 2) Storage driver jobs This is slightly advanced one. Currently, there's no way how to cancel an ongoing storage driver job. For instance, libvirt starts wiping huge file, however user at some point decides to cancel it. And for now, there is no way of doing that (other than just killing 'scub' binary to which we offload the work to). Things get complicated, if we want to share job acquiring code with qemu driver, and report progress (if operation itself is capable of it). 3) libvirt-designer Just an rough idea to extend functionality. No concrete ideas yet. 4) libvirt-snmp Drop autogenerated parts of code, so after each MIB addition one doesn't have to regenerate nearly whole codebase. What do you guys think? Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Google Summer of Code 2013 ideas wiki open
On 2013年02月08日 16:07, Michal Privoznik wrote: On 07.02.2013 16:19, Stefan Hajnoczi wrote: I have created the Google Summer of Code 2013 wiki page where you can add project ideas: http://wiki.qemu.org/Google_Summer_of_Code_2013 Please add project ideas you are willing to mentor. If you have an idea but cannot mentor this year, feel free to add it but please try to find a mentor for it. If you want to be a mentor, please see http://wiki.qemu.org/Google_Summer_of_Code_2013#Information_for_mentors. I've got some ideas, but I just don't know it their size is sufficient. But IIUC, there are 3 levels (beginner, intermediate and advanced), so hopefully the ideas can land in one of them. 1) Virsh auto completion This is something I think will be very useful for nearly all libvirt users. I've proposed the idea a while ago and my sense is we have some consensus how this should work. But I am afraid this is more mechanical work than real programming one. 2) Storage driver jobs This is slightly advanced one. Currently, there's no way how to cancel an ongoing storage driver job. For instance, libvirt starts wiping huge file, however user at some point decides to cancel it. And for now, there is no way of doing that (other than just killing 'scub' binary to which we offload the work to). Things get complicated, if we want to share job acquiring code with qemu driver, and report progress (if operation itself is capable of it). Deserved. We lost this for long time, I filed the bug to track it before, but have not get time to do it yet: https://bugzilla.redhat.com/show_bug.cgi?id=830676 Except this, glusterfs support is another thing I see we need to do sooner or later. I have ever had a glance at the gluterfs, as far as I see, setting up the basic is not complex, but fully support is an advanced job. 3) libvirt-designer Just an rough idea to extend functionality. No concrete ideas yet. 4) libvirt-snmp Drop autogenerated parts of code, so after each MIB addition one doesn't have to regenerate nearly whole codebase. What do you guys think? Michal -- 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
Re: [libvirt] [PATCH 2/3] Rename 'qemuCapsXXX' to 'virQEMUCapsXXX'
On Thu, Feb 07, 2013 at 17:01:50 +, Daniel P. Berrange wrote: > On Wed, Feb 06, 2013 at 04:30:18PM +0100, Jiri Denemark wrote: > > > > I'm not sure if it was intentional or not but the rename of all > > qemuCapsPtr (now virQEMUCapsPtr) to qemuCaps is incomplete. Lots of > > missed instances can be found in src/qemu/qemu_capabilities.[ch] and two > > of them remained in src/qemu/qemu_process.c. > > Hmm, yes that was an oversight. > > Here is a delta-patch for review. ACK with the following patch squashed in: diff --git i/src/qemu/qemu_capabilities.h w/src/qemu/qemu_capabilities.h index 6020d0a..e69d558 100644 --- i/src/qemu/qemu_capabilities.h +++ w/src/qemu/qemu_capabilities.h @@ -177,46 +177,46 @@ typedef struct _virQEMUCapsCache virQEMUCapsCache; typedef virQEMUCapsCache *virQEMUCapsCachePtr; virQEMUCapsPtr virQEMUCapsNew(void); -virQEMUCapsPtr virQEMUCapsNewCopy(virQEMUCapsPtr caps); +virQEMUCapsPtr virQEMUCapsNewCopy(virQEMUCapsPtr qemuCaps); virQEMUCapsPtr virQEMUCapsNewForBinary(const char *binary, const char *libDir, uid_t runUid, gid_t runGid); -int virQEMUCapsProbeQMP(virQEMUCapsPtr caps, +int virQEMUCapsProbeQMP(virQEMUCapsPtr qemuCaps, qemuMonitorPtr mon); -void virQEMUCapsSet(virQEMUCapsPtr caps, +void virQEMUCapsSet(virQEMUCapsPtr qemuCaps, enum virQEMUCapsFlags flag) ATTRIBUTE_NONNULL(1); -void virQEMUCapsSetList(virQEMUCapsPtr caps, ...) ATTRIBUTE_NONNULL(1); +void virQEMUCapsSetList(virQEMUCapsPtr qemuCaps, ...) ATTRIBUTE_NONNULL(1); -void virQEMUCapsClear(virQEMUCapsPtr caps, +void virQEMUCapsClear(virQEMUCapsPtr qemuCaps, enum virQEMUCapsFlags flag) ATTRIBUTE_NONNULL(1); -bool virQEMUCapsGet(virQEMUCapsPtr caps, +bool virQEMUCapsGet(virQEMUCapsPtr qemuCaps, enum virQEMUCapsFlags flag); -char *virQEMUCapsFlagsString(virQEMUCapsPtr caps); +char *virQEMUCapsFlagsString(virQEMUCapsPtr qemuCaps); -const char *virQEMUCapsGetBinary(virQEMUCapsPtr caps); -virArch virQEMUCapsGetArch(virQEMUCapsPtr caps); -unsigned int virQEMUCapsGetVersion(virQEMUCapsPtr caps); -unsigned int virQEMUCapsGetKVMVersion(virQEMUCapsPtr caps); -int virQEMUCapsAddCPUDefinition(virQEMUCapsPtr caps, +const char *virQEMUCapsGetBinary(virQEMUCapsPtr qemuCaps); +virArch virQEMUCapsGetArch(virQEMUCapsPtr qemuCaps); +unsigned int virQEMUCapsGetVersion(virQEMUCapsPtr qemuCaps); +unsigned int virQEMUCapsGetKVMVersion(virQEMUCapsPtr qemuCaps); +int virQEMUCapsAddCPUDefinition(virQEMUCapsPtr qemuCaps, const char *name); -size_t virQEMUCapsGetCPUDefinitions(virQEMUCapsPtr caps, +size_t virQEMUCapsGetCPUDefinitions(virQEMUCapsPtr qemuCaps, char ***names); -size_t virQEMUCapsGetMachineTypes(virQEMUCapsPtr caps, +size_t virQEMUCapsGetMachineTypes(virQEMUCapsPtr qemuCaps, char ***names); -const char *virQEMUCapsGetCanonicalMachine(virQEMUCapsPtr caps, +const char *virQEMUCapsGetCanonicalMachine(virQEMUCapsPtr qemuCaps, const char *name); -int virQEMUCapsGetMachineTypesCaps(virQEMUCapsPtr caps, +int virQEMUCapsGetMachineTypesCaps(virQEMUCapsPtr qemuCaps, size_t *nmachines, virCapsGuestMachinePtr **machines); -bool virQEMUCapsIsValid(virQEMUCapsPtr caps); +bool virQEMUCapsIsValid(virQEMUCapsPtr qemuCaps); virQEMUCapsCachePtr virQEMUCapsCacheNew(const char *libDir, @@ -236,15 +236,15 @@ int virQEMUCapsGetDefaultVersion(virCapsPtr caps, /* Only for use by test suite */ int virQEMUCapsParseHelpStr(const char *qemu, const char *str, -virQEMUCapsPtr caps, +virQEMUCapsPtr qemuCaps, unsigned int *version, unsigned int *is_kvm, unsigned int *kvm_version, bool check_yajl); /* Only for use by test suite */ -int virQEMUCapsParseDeviceStr(virQEMUCapsPtr caps, const char *str); +int virQEMUCapsParseDeviceStr(virQEMUCapsPtr qemuCaps, const char *str); VIR_ENUM_DECL(virQEMUCaps); -bool virQEMUCapsUsedQMP(virQEMUCapsPtr caps); +bool virQEMUCapsUsedQMP(virQEMUCapsPtr qemuCaps); #endif /* __QEMU_CAPABILITIES_H__*/ -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH V3 1/2] Intorduce file descriptor set for QEMU domains
On Thu, Feb 07, 2013 at 05:26:10PM -0500, Stefan Berger wrote: > On 02/07/2013 12:58 PM, Daniel P. Berrange wrote: > >On Thu, Feb 07, 2013 at 12:56:45PM -0500, Stefan Berger wrote: > >>On 02/07/2013 10:12 AM, Daniel P. Berrange wrote: > >>>On Mon, Feb 04, 2013 at 07:12:37PM -0500, Stefan Berger wrote: > Create field in virDomainDeviceInfo structure to hold > file descriptor set the device is associated with. Have > the number written into the Device Info XML and parsed > from the XML. > Remember the next-toy use file descriptor set in the QEMU > private domain structure. > Upon libvirt restart determine the maximum file descriptor > set used in the Device Info XML and remember the > next-to-use file descriptor set in the QEMU private domain > structure. > > Upon termination of a domain, reset the next-to-use file > descriptor set to its initial value '1'. > > Stefan Berger > > --- > src/conf/capabilities.h |5 - > src/conf/domain_conf.c | 20 +--- > src/conf/domain_conf.h |1 + > src/lxc/lxc_domain.c|3 ++- > src/qemu/qemu_domain.c | 23 ++- > src/qemu/qemu_domain.h |2 ++ > src/qemu/qemu_process.c |2 ++ > 7 files changed, 50 insertions(+), 6 deletions(-) > >>>I really don't like the fact that we're modifying the virDomainDef > >>>structs, which are intended to hold static config description, to > >>>also hold QEMU specific dynamic state. We did this in the past > >>>before we had the per-driver 'privateData' associated with > >>>virDomainObjPtr instances. > >>In these patches we are storing the fdset that's in use by a > >>particular device in the virDomainDeviceInfo structure and are > >>persisting that fdset number next to the device's alias into XML > >>written into /var/run/libvirt/qemu to be able to restore it upon > >>libvirt restart. Now we would either need a similar per-device > >>structure in parallel to those virDomainDeviceInfo structures or > >>connect a QEMU private data structure to that virDomainDeviceInfo > >>structure for storing the dynamic data and to keep reference back to > >>the device. Not sure what's the best path ... > >Just have a hashtable mapping device alias names -> fd set numbers > >in the qemuDomainObjPrivatePtr, and write that out in the QEMU > >private XML. There's no need to directly mirror the structs. > > > > you agree to this XML ? > > > > > > Yes, sort sort of thing looks better to me 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]Fix bugs of Sheepdog storage driver
On Fri, Feb 8, 2013 at 2:00 PM, Osier Yang wrote: [...] > > Btw, if you look further more on virStorageBackendSheepdogRefreshVol, > you will see it has bugs too. It can be another patch though. > Thanks, i will fix some other bugs ;-) -- Thanks Harry Wei -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] fix some bugs of sheepdog storage driver
Signed-off-by: Harry Wei --- src/storage/storage_backend_sheepdog.c | 10 ++ 1 files changed, 10 insertions(+), 0 deletions(-) diff --git a/src/storage/storage_backend_sheepdog.c b/src/storage/storage_backend_sheepdog.c index 218284d..1ce5aa4 100644 --- a/src/storage/storage_backend_sheepdog.c +++ b/src/storage/storage_backend_sheepdog.c @@ -120,6 +120,8 @@ virStorageBackendSheepdogRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED, virCommandPtr cmd; cmd = virCommandNewArgList(COLLIE, "node", "info", "-r", NULL); +if (cmd == NULL) +return -1; virStorageBackendSheepdogAddHostArg(cmd, pool); virCommandSetOutputBuffer(cmd, &output); ret = virCommandRun(cmd, NULL); @@ -142,6 +144,8 @@ virStorageBackendSheepdogDeleteVol(virConnectPtr conn ATTRIBUTE_UNUSED, virCheckFlags(0, -1); virCommandPtr cmd = virCommandNewArgList(COLLIE, "vdi", "delete", vol->name, NULL); +if (cmd == NULL) +return -1; virStorageBackendSheepdogAddHostArg(cmd, pool); int ret = virCommandRun(cmd, NULL); @@ -165,6 +169,8 @@ virStorageBackendSheepdogCreateVol(virConnectPtr conn ATTRIBUTE_UNUSED, } virCommandPtr cmd = virCommandNewArgList(COLLIE, "vdi", "create", vol->name, NULL); +if (cmd == NULL) +return -1; virCommandAddArgFormat(cmd, "%llu", vol->capacity); virStorageBackendSheepdogAddHostArg(cmd, pool); if (virCommandRun(cmd, NULL) < 0) @@ -251,6 +257,8 @@ virStorageBackendSheepdogRefreshVol(virConnectPtr conn ATTRIBUTE_UNUSED, char *output = NULL; virCommandPtr cmd = virCommandNewArgList(COLLIE, "vdi", "list", vol->name, "-r", NULL); +if (cmd == NULL) +return -1; virStorageBackendSheepdogAddHostArg(cmd, pool); virCommandSetOutputBuffer(cmd, &output); ret = virCommandRun(cmd, NULL); @@ -293,6 +301,8 @@ virStorageBackendSheepdogResizeVol(virConnectPtr conn ATTRIBUTE_UNUSED, virCheckFlags(0, -1); virCommandPtr cmd = virCommandNewArgList(COLLIE, "vdi", "resize", vol->name, NULL); +if (cmd == NULL) +return -1; virCommandAddArgFormat(cmd, "%llu", capacity); virStorageBackendSheepdogAddHostArg(cmd, pool); int ret = virCommandRun(cmd, NULL); -- 1.7.0.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] fix some bugs of sheepdog storage driver
Don't try to do other jobs if get cmd NULL. Signed-off-by: Harry Wei --- src/storage/storage_backend_sheepdog.c | 10 ++ 1 files changed, 10 insertions(+), 0 deletions(-) diff --git a/src/storage/storage_backend_sheepdog.c b/src/storage/storage_backend_sheepdog.c index 218284d..1ce5aa4 100644 --- a/src/storage/storage_backend_sheepdog.c +++ b/src/storage/storage_backend_sheepdog.c @@ -120,6 +120,8 @@ virStorageBackendSheepdogRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED, virCommandPtr cmd; cmd = virCommandNewArgList(COLLIE, "node", "info", "-r", NULL); +if (cmd == NULL) +return -1; virStorageBackendSheepdogAddHostArg(cmd, pool); virCommandSetOutputBuffer(cmd, &output); ret = virCommandRun(cmd, NULL); @@ -142,6 +144,8 @@ virStorageBackendSheepdogDeleteVol(virConnectPtr conn ATTRIBUTE_UNUSED, virCheckFlags(0, -1); virCommandPtr cmd = virCommandNewArgList(COLLIE, "vdi", "delete", vol->name, NULL); +if (cmd == NULL) +return -1; virStorageBackendSheepdogAddHostArg(cmd, pool); int ret = virCommandRun(cmd, NULL); @@ -165,6 +169,8 @@ virStorageBackendSheepdogCreateVol(virConnectPtr conn ATTRIBUTE_UNUSED, } virCommandPtr cmd = virCommandNewArgList(COLLIE, "vdi", "create", vol->name, NULL); +if (cmd == NULL) +return -1; virCommandAddArgFormat(cmd, "%llu", vol->capacity); virStorageBackendSheepdogAddHostArg(cmd, pool); if (virCommandRun(cmd, NULL) < 0) @@ -251,6 +257,8 @@ virStorageBackendSheepdogRefreshVol(virConnectPtr conn ATTRIBUTE_UNUSED, char *output = NULL; virCommandPtr cmd = virCommandNewArgList(COLLIE, "vdi", "list", vol->name, "-r", NULL); +if (cmd == NULL) +return -1; virStorageBackendSheepdogAddHostArg(cmd, pool); virCommandSetOutputBuffer(cmd, &output); ret = virCommandRun(cmd, NULL); @@ -293,6 +301,8 @@ virStorageBackendSheepdogResizeVol(virConnectPtr conn ATTRIBUTE_UNUSED, virCheckFlags(0, -1); virCommandPtr cmd = virCommandNewArgList(COLLIE, "vdi", "resize", vol->name, NULL); +if (cmd == NULL) +return -1; virCommandAddArgFormat(cmd, "%llu", capacity); virStorageBackendSheepdogAddHostArg(cmd, pool); int ret = virCommandRun(cmd, NULL); -- 1.7.0.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] fix some bugs of sheepdog storage driver
On Fri, Feb 08, 2013 at 06:49:47PM +0800, harryxi...@gmail.com wrote: > Don't try to do other jobs if get cmd NULL. > > Signed-off-by: Harry Wei > > --- > src/storage/storage_backend_sheepdog.c | 10 ++ > 1 files changed, 10 insertions(+), 0 deletions(-) > > diff --git a/src/storage/storage_backend_sheepdog.c > b/src/storage/storage_backend_sheepdog.c > index 218284d..1ce5aa4 100644 > --- a/src/storage/storage_backend_sheepdog.c > +++ b/src/storage/storage_backend_sheepdog.c > @@ -120,6 +120,8 @@ virStorageBackendSheepdogRefreshPool(virConnectPtr conn > ATTRIBUTE_UNUSED, > virCommandPtr cmd; > > cmd = virCommandNewArgList(COLLIE, "node", "info", "-r", NULL); > +if (cmd == NULL) > +return -1; NACK, the virCommand APIs are designed explicitly so that you do *not* need to check for NULL in this way. This page has a description of the usage of virCommand APIs http://libvirt.org/internals/command.html#initial 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] fix some bugs of sheepdog storage driver
On Fri, Feb 8, 2013 at 7:11 PM, Daniel P. Berrange wrote: > On Fri, Feb 08, 2013 at 06:49:47PM +0800, harryxi...@gmail.com wrote: >> Don't try to do other jobs if get cmd NULL. >> >> Signed-off-by: Harry Wei >> >> --- >> src/storage/storage_backend_sheepdog.c | 10 ++ >> 1 files changed, 10 insertions(+), 0 deletions(-) >> >> diff --git a/src/storage/storage_backend_sheepdog.c >> b/src/storage/storage_backend_sheepdog.c >> index 218284d..1ce5aa4 100644 >> --- a/src/storage/storage_backend_sheepdog.c >> +++ b/src/storage/storage_backend_sheepdog.c >> @@ -120,6 +120,8 @@ virStorageBackendSheepdogRefreshPool(virConnectPtr conn >> ATTRIBUTE_UNUSED, >> virCommandPtr cmd; >> >> cmd = virCommandNewArgList(COLLIE, "node", "info", "-r", NULL); >> +if (cmd == NULL) >> +return -1; > > NACK, the virCommand APIs are designed explicitly so that you do *not* > need to check for NULL in this way. This page has a description of the > usage of virCommand APIs > > http://libvirt.org/internals/command.html#initial > Thanks, i see. -- Thanks Harry Wei -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] Why not add snapshot operations for Sheepdog storage driver in Libvirt
Hi all, Virsh has following operations about snapshot. Snapshot (help keyword 'snapshot') snapshot-createCreate a snapshot from XML snapshot-create-as Create a snapshot from a set of args snapshot-current Get or set the current snapshot snapshot-deleteDelete a domain snapshot snapshot-dumpxml Dump XML for a domain snapshot snapshot-edit edit XML for a snapshot snapshot-info snapshot information snapshot-list List snapshots for a domain snapshot-parentGet the name of the parent of a snapshot snapshot-revertRevert a domain to a snapshot But i cannot find relevant realization in Sheepdog storage driver in Libvirt. I am sure Sheepdog storage driver, now, cannot support snapshot operations in Libvirt. I wonder whether Sheepdog can realize snapshot operations in Libvirt? Maybe i can finish these jobs. I am not clear about how to let Sheepdog storage driver support snapshot operations. Could anyone give me some suggestions? Thanks in advance. -- Thanks Harry Wei -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] fix some bugs of sheepdog storage driver
On 02/08/2013 03:48 AM, harryxi...@gmail.com wrote: > Signed-off-by: Harry Wei > > --- > src/storage/storage_backend_sheepdog.c | 10 ++ > 1 files changed, 10 insertions(+), 0 deletions(-) > > diff --git a/src/storage/storage_backend_sheepdog.c > b/src/storage/storage_backend_sheepdog.c > index 218284d..1ce5aa4 100644 > --- a/src/storage/storage_backend_sheepdog.c > +++ b/src/storage/storage_backend_sheepdog.c > @@ -120,6 +120,8 @@ virStorageBackendSheepdogRefreshPool(virConnectPtr conn > ATTRIBUTE_UNUSED, > virCommandPtr cmd; > > cmd = virCommandNewArgList(COLLIE, "node", "info", "-r", NULL); > +if (cmd == NULL) > +return -1; > virStorageBackendSheepdogAddHostArg(cmd, pool); > virCommandSetOutputBuffer(cmd, &output); > ret = virCommandRun(cmd, NULL); Not needed. If cmd is NULL, virCommandRun will gracefully report the OOM message and return -1. This patch can be dropped. -- 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] Why not add snapshot operations for Sheepdog storage driver in Libvirt
On 02/08/2013 04:56 AM, harryxiyou wrote: > Hi all, > > Virsh has following operations about snapshot. > > Snapshot (help keyword 'snapshot') > snapshot-createCreate a snapshot from XML > snapshot-create-as Create a snapshot from a set of args > snapshot-current Get or set the current snapshot > snapshot-deleteDelete a domain snapshot > snapshot-dumpxml Dump XML for a domain snapshot > snapshot-edit edit XML for a snapshot > snapshot-info snapshot information > snapshot-list List snapshots for a domain > snapshot-parentGet the name of the parent of a snapshot > snapshot-revertRevert a domain to a snapshot These commands are related to domain snapshots. > > But i cannot find relevant realization in Sheepdog storage driver in Libvirt. That would be a storage volume snapshot, which is a different task. Remember, a domain snapshot may include storage volume snapshots, and may also include memory state snapshots, which makes it a more comprehensive task; but also that some of the work of a domain snapshot is offloaded to qemu, whereas storage volume snapshot manipulation would have to be done in libvirt independently of qemu. At one point I proposed API for doing storage volume snapshot manipulation, but it never became a pressing need, so no one has implemented it yet. https://www.redhat.com/archives/libvir-list/2011-June/msg00761.html > I am sure Sheepdog storage driver, now, cannot support snapshot operations > in Libvirt. I wonder whether Sheepdog can realize snapshot operations > in Libvirt? Not just sheepdog, but qcow2 storage volume management would also benefit from storage volume snapshot commands. -- 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 0/4] Fix problems of shared disk management
This fixes several problems of shared disk management, mainly about shared cdrom or floppy disk. Osier Yang (4): qemu: Add checking in helpers for sgio setting qemu: Merge qemuCheckSharedDisk into qemuAddSharedDisk qemu: Don't remove hash entry of other domains qemu: Move shared disk entry adding and unpriv_sgio seting src/qemu/qemu_conf.c | 73 ++-- src/qemu/qemu_conf.h |5 +- src/qemu/qemu_driver.c| 55 +++- src/qemu/qemu_hotplug.c |6 ++- src/qemu/qemu_hotplug.h |3 +- src/qemu/qemu_migration.c | 14 +++--- src/qemu/qemu_process.c | 101 - src/qemu/qemu_process.h |6 +-- 8 files changed, 149 insertions(+), 114 deletions(-) -- 1.7.7.6 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/4] qemu: Add checking in helpers for sgio setting
This moves the checking into the helpers, to avoid the
callers missing the checking.
---
src/qemu/qemu_conf.c| 20
src/qemu/qemu_conf.h|4 ++--
src/qemu/qemu_driver.c | 18 +++---
src/qemu/qemu_process.c | 21 -
4 files changed, 37 insertions(+), 26 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 17f7d45..69ba710 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -726,12 +726,20 @@ qemuGetSharedDiskKey(const char *disk_path)
*/
int
qemuAddSharedDisk(virHashTablePtr sharedDisks,
- const char *disk_path)
+ virDomainDiskDefPtr disk)
{
size_t *ref = NULL;
char *key = NULL;
-if (!(key = qemuGetSharedDiskKey(disk_path)))
+/* Currently the only conflicts we have to care about
+ * for the shared disk is "sgio" setting, which is only
+ * valid for block disk.
+ */
+if (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK ||
+!disk->shared || !disk->src)
+return 0;
+
+if (!(key = qemuGetSharedDiskKey(disk->src)))
return -1;
if ((ref = virHashLookup(sharedDisks, key))) {
@@ -755,12 +763,16 @@ qemuAddSharedDisk(virHashTablePtr sharedDisks,
*/
int
qemuRemoveSharedDisk(virHashTablePtr sharedDisks,
- const char *disk_path)
+ virDomainDiskDefPtr disk)
{
size_t *ref = NULL;
char *key = NULL;
-if (!(key = qemuGetSharedDiskKey(disk_path)))
+if (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK ||
+!disk->shared || !disk->src)
+return 0;
+
+if (!(key = qemuGetSharedDiskKey(disk->src)))
return -1;
if (!(ref = virHashLookup(sharedDisks, key))) {
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index 60c4109..8e84bcf 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -270,11 +270,11 @@ void qemuDriverCloseCallbackRunAll(virQEMUDriverPtr
driver,
virConnectPtr conn);
int qemuAddSharedDisk(virHashTablePtr sharedDisks,
- const char *disk_path)
+ virDomainDiskDefPtr disk)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
int qemuRemoveSharedDisk(virHashTablePtr sharedDisks,
- const char *disk_path)
+ virDomainDiskDefPtr disk)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
char * qemuGetSharedDiskKey(const char *disk_path)
ATTRIBUTE_NONNULL(1);
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 979a027..043efd3 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5855,11 +5855,9 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
}
if (ret == 0) {
-if (disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK && disk->shared) {
-if (qemuAddSharedDisk(driver->sharedDisks, disk->src) < 0)
-VIR_WARN("Failed to add disk '%s' to shared disk table",
- disk->src);
-}
+if (qemuAddSharedDisk(driver->sharedDisks, disk) < 0)
+VIR_WARN("Failed to add disk '%s' to shared disk table",
+ disk->src);
if (qemuSetUnprivSGIO(disk) < 0)
VIR_WARN("Failed to set unpriv_sgio of disk '%s'", disk->src);
@@ -5980,12 +5978,10 @@ qemuDomainDetachDeviceDiskLive(virQEMUDriverPtr driver,
break;
}
-if (ret == 0 &&
-disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK &&
-disk->shared) {
-if (qemuRemoveSharedDisk(driver->sharedDisks, disk->src) < 0)
- VIR_WARN("Failed to remove disk '%s' from shared disk table",
- disk->src);
+if (ret == 0) {
+if (qemuRemoveSharedDisk(driver->sharedDisks, disk) < 0)
+VIR_WARN("Failed to remove disk '%s' from shared disk table",
+ disk->src);
}
return ret;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 30f923a..bc171a4 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3453,6 +3453,13 @@ qemuSetUnprivSGIO(virDomainDiskDefPtr disk)
{
int val = -1;
+/* "sgio" is only valid for block disk; cdrom
+ * and floopy disk can have empty source.
+ */
+if (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK ||
+!disk->src)
+return 0;
+
if (disk->sgio)
val = (disk->sgio == VIR_DOMAIN_DISK_SGIO_UNFILTERED);
@@ -3875,13 +3882,11 @@ int qemuProcessStart(virConnectPtr conn,
_("Raw I/O is not supported on this platform"));
#endif
-if (disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK && disk->shared) {
-if (qemuAddSharedDisk(driver->sharedDisks, disk->src) < 0)
-goto cleanup;
+if (qemuAddSharedDisk(driver->sharedDisks, disk) < 0)
+goto cleanup;
-if (qemuCheckSharedDisk(driver->sharedDisks, disk) < 0)
-goto cleanup;
-}
+
[libvirt] [PATCH 2/4] qemu: Merge qemuCheckSharedDisk into qemuAddSharedDisk
Based on moving various checking into qemuAddSharedDisk, this
avoids the caller using it in wrong ways.
---
src/qemu/qemu_conf.c| 50
src/qemu/qemu_driver.c |5
src/qemu/qemu_process.c | 53 ---
src/qemu/qemu_process.h |3 --
4 files changed, 50 insertions(+), 61 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 69ba710..3eeea4b 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -721,6 +721,53 @@ qemuGetSharedDiskKey(const char *disk_path)
return key;
}
+/* Check if a shared disk's setting conflicts with the conf
+ * used by other domain(s). Currently only checks the sgio
+ * setting. Note that this should only be called for disk with
+ * block source.
+ *
+ * Returns 0 if no conflicts, otherwise returns -1.
+ */
+static int
+qemuCheckSharedDisk(virHashTablePtr sharedDisks,
+virDomainDiskDefPtr disk)
+{
+int val;
+size_t *ref = NULL;
+char *key = NULL;
+int ret = 0;
+
+if (!(key = qemuGetSharedDiskKey(disk->src)))
+return -1;
+
+/* It can't be conflict if no other domain is
+ * is sharing it.
+ */
+if (!(ref = virHashLookup(sharedDisks, key)))
+goto cleanup;
+
+if (virGetDeviceUnprivSGIO(disk->src, NULL, &val) < 0) {
+ret = -1;
+goto cleanup;
+}
+
+if ((val == 0 &&
+ (disk->sgio == VIR_DOMAIN_DISK_SGIO_FILTERED ||
+ disk->sgio == VIR_DOMAIN_DISK_SGIO_DEFAULT)) ||
+(val == 1 &&
+ disk->sgio == VIR_DOMAIN_DISK_SGIO_UNFILTERED))
+goto cleanup;
+
+virReportError(VIR_ERR_OPERATION_INVALID,
+ _("sgio of shared disk '%s' conflicts with other "
+ "active domains"), disk->src);
+ret = -1;
+
+cleanup:
+VIR_FREE(key);
+return ret;
+}
+
/* Increase ref count if the entry already exists, otherwise
* add a new entry.
*/
@@ -739,6 +786,9 @@ qemuAddSharedDisk(virHashTablePtr sharedDisks,
!disk->shared || !disk->src)
return 0;
+if (qemuCheckSharedDisk(sharedDisks, disk) < 0)
+return -1;
+
if (!(key = qemuGetSharedDiskKey(disk->src)))
return -1;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 043efd3..1dc9789 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5798,11 +5798,6 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
goto end;
}
-if (disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK &&
-disk->shared &&
-(qemuCheckSharedDisk(driver->sharedDisks, disk) < 0))
-goto end;
-
if (qemuDomainDetermineDiskChain(driver, disk, false) < 0)
goto end;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index bc171a4..1e0876c 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3483,56 +3483,6 @@ qemuSetUnprivSGIO(virDomainDiskDefPtr disk)
return 0;
}
-/* Check if a shared disk's setting conflicts with the conf
- * used by other domain(s). Currently only checks the sgio
- * setting. Note that this should only be called for disk with
- * block source.
- *
- * Returns 0 if no conflicts, otherwise returns -1.
- */
-int
-qemuCheckSharedDisk(virHashTablePtr sharedDisks,
-virDomainDiskDefPtr disk)
-{
-int val;
-size_t *ref = NULL;
-char *key = NULL;
-int ret = 0;
-
-if (!(key = qemuGetSharedDiskKey(disk->src)))
-return -1;
-
-/* It can't be conflict if no other domain is
- * is sharing it.
- */
-if (!(ref = virHashLookup(sharedDisks, key)))
-goto cleanup;
-
-if (ref == (void *)0x1)
-goto cleanup;
-
-if (virGetDeviceUnprivSGIO(disk->src, NULL, &val) < 0) {
-ret = -1;
-goto cleanup;
-}
-
-if ((val == 0 &&
- (disk->sgio == VIR_DOMAIN_DISK_SGIO_FILTERED ||
- disk->sgio == VIR_DOMAIN_DISK_SGIO_DEFAULT)) ||
-(val == 1 &&
- disk->sgio == VIR_DOMAIN_DISK_SGIO_UNFILTERED))
-goto cleanup;
-
-virReportError(VIR_ERR_OPERATION_INVALID,
- _("sgio of shared disk '%s' conflicts with other "
- "active domains"), disk->src);
-ret = -1;
-
-cleanup:
-VIR_FREE(key);
-return ret;
-}
-
int qemuProcessStart(virConnectPtr conn,
virQEMUDriverPtr driver,
virDomainObjPtr vm,
@@ -3885,9 +3835,6 @@ int qemuProcessStart(virConnectPtr conn,
if (qemuAddSharedDisk(driver->sharedDisks, disk) < 0)
goto cleanup;
-if (qemuCheckSharedDisk(driver->sharedDisks, disk) < 0)
-goto cleanup;
-
if (qemuSetUnprivSGIO(disk) < 0)
goto cleanup;
}
diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h
index 2dc8041..7c620d4 100644
--- a/src/qemu/qemu_process.h
+++ b/src/qemu/qemu_process.h
@@ -100,7 +100,4 @@ virBitmapPtr qemuPrepareCpuma
[libvirt] [PATCH 3/4] qemu: Don't remove hash entry of other domains
qemuProcessStart invokes qemuProcessStop when fails, to avoid
removing hash entry which belongs to other domain(s), this introduces
a new virBitmapPtr argument for qemuProcessStop. And qemuProcessStart
sets the bit for the disk only if it's successfully added into the
hash table. Thus if the argument is provided for qemuProcessStop, it
can't remove the hash entry belongs to other domain(s).
---
src/qemu/qemu_conf.c |5 -
src/qemu/qemu_conf.h |3 ++-
src/qemu/qemu_driver.c| 21 +++--
src/qemu/qemu_migration.c | 14 +++---
src/qemu/qemu_process.c | 37 +
src/qemu/qemu_process.h |3 ++-
6 files changed, 55 insertions(+), 28 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 3eeea4b..a6162b6 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -773,7 +773,8 @@ cleanup:
*/
int
qemuAddSharedDisk(virHashTablePtr sharedDisks,
- virDomainDiskDefPtr disk)
+ virDomainDiskDefPtr disk,
+ int *added)
{
size_t *ref = NULL;
char *key = NULL;
@@ -804,6 +805,8 @@ qemuAddSharedDisk(virHashTablePtr sharedDisks,
}
}
+if (added)
+*added = 1;
VIR_FREE(key);
return 0;
}
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index 8e84bcf..b8b5275 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -270,7 +270,8 @@ void qemuDriverCloseCallbackRunAll(virQEMUDriverPtr driver,
virConnectPtr conn);
int qemuAddSharedDisk(virHashTablePtr sharedDisks,
- virDomainDiskDefPtr disk)
+ virDomainDiskDefPtr disk,
+ int *added)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
int qemuRemoveSharedDisk(virHashTablePtr sharedDisks,
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 1dc9789..03fe526 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2098,7 +2098,7 @@ qemuDomainDestroyFlags(virDomainPtr dom,
goto endjob;
}
-qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_DESTROYED, 0);
+qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_DESTROYED, 0, NULL);
event = virDomainEventNewFromObj(vm,
VIR_DOMAIN_EVENT_STOPPED,
VIR_DOMAIN_EVENT_STOPPED_DESTROYED);
@@ -2944,7 +2944,7 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver,
virDomainPtr dom,
goto endjob;
/* Shut it down */
-qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_SAVED, 0);
+qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_SAVED, 0, NULL);
virDomainAuditStop(vm, "saved");
event = virDomainEventNewFromObj(vm,
VIR_DOMAIN_EVENT_STOPPED,
@@ -3393,7 +3393,7 @@ static int qemuDomainCoreDump(virDomainPtr dom,
endjob:
if ((ret == 0) && (flags & VIR_DUMP_CRASH)) {
-qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_CRASHED, 0);
+qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_CRASHED, 0, NULL);
virDomainAuditStop(vm, "crashed");
event = virDomainEventNewFromObj(vm,
VIR_DOMAIN_EVENT_STOPPED,
@@ -4902,7 +4902,7 @@ qemuDomainSaveImageStartVM(virConnectPtr conn,
}
if (virCommandWait(cmd, NULL) < 0) {
-qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, 0);
+qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, 0, NULL);
ret = -1;
}
VIR_DEBUG("Decompression binary stderr: %s", NULLSTR(errbuf));
@@ -5850,7 +5850,7 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
}
if (ret == 0) {
-if (qemuAddSharedDisk(driver->sharedDisks, disk) < 0)
+if (qemuAddSharedDisk(driver->sharedDisks, disk, NULL) < 0)
VIR_WARN("Failed to add disk '%s' to shared disk table",
disk->src);
@@ -10677,7 +10677,7 @@ qemuDomainSnapshotCreateActiveInternal(virConnectPtr
conn,
if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_HALT) {
event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED,
VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT);
-qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, 0);
+qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, 0, NULL);
virDomainAuditStop(vm, "from-snapshot");
/* We already filtered the _HALT flag for persistent domains
* only, so this end job never drops the last reference. */
@@ -11232,7 +11232,7 @@ qemuDomainSnapshotCreateActiveExternal(virConnectPtr
conn,
event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED,
VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT);
-qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, 0);
+qemuProcessStop(driver,
[libvirt] [PATCH 4/4] qemu: Move shared disk entry adding and unpriv_sgio seting
The disk def could be free'ed by qemuDomainChangeEjectableMedia
for cdrom or floppy disk. This moves the adding and setting before
the attaching takes place. And for cdrom floppy disk, if the
change is ejecting, removing the existed hash entry for it.
---
src/qemu/qemu_driver.c | 23 +--
src/qemu/qemu_hotplug.c |6 +-
src/qemu/qemu_hotplug.h |3 ++-
3 files changed, 20 insertions(+), 12 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 03fe526..4aad42f 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5789,6 +5789,7 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
{
virDomainDiskDefPtr disk = dev->data.disk;
virCgroupPtr cgroup = NULL;
+int eject, added;
int ret = -1;
if (disk->driverName != NULL && !STREQ(disk->driverName, "qemu")) {
@@ -5798,6 +5799,12 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
goto end;
}
+if (qemuAddSharedDisk(driver->sharedDisks, disk, &added) < 0)
+goto end;
+
+if (qemuSetUnprivSGIO(disk) < 0)
+goto end;
+
if (qemuDomainDetermineDiskChain(driver, disk, false) < 0)
goto end;
@@ -5814,7 +5821,7 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
switch (disk->device) {
case VIR_DOMAIN_DISK_DEVICE_CDROM:
case VIR_DOMAIN_DISK_DEVICE_FLOPPY:
-ret = qemuDomainChangeEjectableMedia(driver, vm, disk, false);
+ret = qemuDomainChangeEjectableMedia(driver, vm, disk, false, &eject);
break;
case VIR_DOMAIN_DISK_DEVICE_DISK:
case VIR_DOMAIN_DISK_DEVICE_LUN:
@@ -5843,22 +5850,18 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
break;
}
+if (ret == 0 && eject)
+ignore_value(qemuRemoveSharedDisk(driver->sharedDisks, disk));
+
if (ret != 0 && cgroup) {
if (qemuTeardownDiskCgroup(vm, cgroup, disk) < 0)
VIR_WARN("Failed to teardown cgroup for disk path %s",
NULLSTR(disk->src));
}
-if (ret == 0) {
-if (qemuAddSharedDisk(driver->sharedDisks, disk, NULL) < 0)
-VIR_WARN("Failed to add disk '%s' to shared disk table",
- disk->src);
-
-if (qemuSetUnprivSGIO(disk) < 0)
-VIR_WARN("Failed to set unpriv_sgio of disk '%s'", disk->src);
-}
-
end:
+if (ret != 0 && added)
+ignore_value(qemuRemoveSharedDisk(driver->sharedDisks, disk))
if (cgroup)
virCgroupFree(&cgroup);
return ret;
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 98912bf..18aca47 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -53,7 +53,8 @@
int qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
virDomainObjPtr vm,
virDomainDiskDefPtr disk,
- bool force)
+ bool force,
+ int *eject)
{
virDomainDiskDefPtr origdisk = NULL;
int i;
@@ -93,6 +94,9 @@ int qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
goto cleanup;
}
+if (eject && origdisk->src && !disk->src)
+*eject = 1;
+
if (virDomainLockDiskAttach(driver->lockManager, cfg->uri,
vm, disk) < 0)
goto cleanup;
diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h
index 8f01d23..fc0532a 100644
--- a/src/qemu/qemu_hotplug.h
+++ b/src/qemu/qemu_hotplug.h
@@ -31,7 +31,8 @@
int qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
virDomainObjPtr vm,
virDomainDiskDefPtr disk,
- bool force);
+ bool force,
+ int *eject);
int qemuDomainCheckEjectableMedia(virQEMUDriverPtr driver,
virDomainObjPtr vm,
enum qemuDomainAsyncJob asyncJob);
--
1.7.7.6
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] virsh-snapshot: Fix xpath query to determine snapshot state
The query didn't match the external state correctly for offline internal
snapshots.
---
tools/virsh-snapshot.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c
index ba57059..66776e2 100644
--- a/tools/virsh-snapshot.c
+++ b/tools/virsh-snapshot.c
@@ -983,8 +983,8 @@ cmdSnapshotInfo(vshControl *ctl, const vshCmd *cmd)
* external snapshot. */
switch (virXPathBoolean("boolean(/domainsnapshot/memory)", ctxt)) {
case 1:
-external =
virXPathBoolean("boolean(/domainsnapshot/memory/@snapshot=external "
- "|
/domainsnapshot/disks/disk/@snapshot=external)",
+external =
virXPathBoolean("boolean(/domainsnapshot/memory[@snapshot='external'] "
+ "|
/domainsnapshot/disks/disk[@snapshot='external'])",
ctxt);
break;
case 0:
--
1.8.1.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH V3 1/2] Intorduce file descriptor set for QEMU domains
On 02/08/2013 05:22 AM, Daniel P. Berrange wrote: On Thu, Feb 07, 2013 at 05:26:10PM -0500, Stefan Berger wrote: you agree to this XML ? Yes, sort sort of thing looks better to me Great. And v4 came out really good :-) Stefan -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virsh-snapshot: Fix xpath query to determine snapshot state
On Fri, Feb 08, 2013 at 14:26:28 +0100, Peter Krempa wrote:
> The query didn't match the external state correctly for offline internal
> snapshots.
> ---
> tools/virsh-snapshot.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c
> index ba57059..66776e2 100644
> --- a/tools/virsh-snapshot.c
> +++ b/tools/virsh-snapshot.c
> @@ -983,8 +983,8 @@ cmdSnapshotInfo(vshControl *ctl, const vshCmd *cmd)
> * external snapshot. */
> switch (virXPathBoolean("boolean(/domainsnapshot/memory)", ctxt)) {
> case 1:
> -external =
> virXPathBoolean("boolean(/domainsnapshot/memory/@snapshot=external "
> - "|
> /domainsnapshot/disks/disk/@snapshot=external)",
> +external =
> virXPathBoolean("boolean(/domainsnapshot/memory[@snapshot='external'] "
> + "|
> /domainsnapshot/disks/disk[@snapshot='external'])",
> ctxt);
> break;
> case 0:
ACK, I wonder what did the old XPath actually do :-)
Jirka
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] network_conf.c: Free xmlDoc after use
The virNetworkObjUpdateParseFile() function was not freeing the xml variable, leaving us with a memory leak. --- src/conf/network_conf.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index c93916d..3604ff7 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -1895,6 +1895,7 @@ virNetworkObjUpdateParseFile(const char *filename, ret = 0; cleanup: +xmlFreeDoc(xml); xmlXPathFreeContext(ctxt); return ret; } -- 1.8.0.2 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 00/11] Revisit xen driver Coverity cleanup changes
On 01/30/2013 01:51 PM, John Ferlan wrote:
> src/xen/xen_hypervisor.c | 220 --
> src/xen/xen_inotify.c| 48
> src/xen/xend_internal.c | 303
> ++-
> src/xen/xm_internal.c| 195 ++
> src/xen/xs_internal.c| 239 +++--
> 5 files changed, 325 insertions(+), 680 deletions(-)
>
Thanks for the reviews Osier and Jim. Rather than respond to each, I'll
summarize the consistent points. If I should send a v2, let me know.
w/r/t PrivatePtr on one line:
I was trying to go with the 80 column rule. For 99% they could fit on
one line if I removed one extra space. For a couple, spanning 2 lines
kept the 80 columns in effect. In looking at other drivers - I don't
think any of them typecast the *PrivatePtr)->privateData, so I
could remove that too. I left them in only because they were there.
w/r/t function headers:
I was trying to be consistent amongst all the files and then more in
general with other drivers. I kept to the concept that if a header fits
within 80 columns then don't break up the line. I changed the files
where a header wouldn't fit into 80 columns to be:
function(type arg,
type2 arg2,
type3 arg3,
type4 arg4)
{
}
w/r/t: domain->name:
I wasn't sure with this one, so I had left it in. I removed them.
There are also checks for "domain->id != -1" - any thoughts on those?
Those seem to be running or not checks, right? Seems as though the
virDomainObjIsActive() or hvirCheckFlags() is used elsewhere.
What about priv->handle in xen_hypervisor.c? Seems as though open will
set it and everyone else checks it. Given how these drivers call between
each other, I'm inclined to leave as it, but I'm not against removing
them either.
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] virCommand: Don't misuse the eventloop for async IO
Currently, if a command wants to do asynchronous IO, a callback
is registered in the libvirtd eventloop to handle writes and
reads. However, there's a race in virCommandWait. The eventloop
may already be executing the callback, while virCommandWait is
mangling internal state of virCommand. To deal with it, we need
to either introduce locking or spawn a separate thread where we
poll() on stdio from child. The former, however, requires to
unlock all mutexes held, as the event loop may execute other
callbacks which tries to lock one of the mutexes, deadlock and
thus never wake us up. So it's safer to spawn a separate thread.
---
This is an alternative to:
https://www.redhat.com/archives/libvir-list/2013-February/msg00352.html
src/util/vircommand.c | 342 +++---
1 file changed, 76 insertions(+), 266 deletions(-)
diff --git a/src/util/vircommand.c b/src/util/vircommand.c
index db7dbca..160f4c2 100644
--- a/src/util/vircommand.c
+++ b/src/util/vircommand.c
@@ -42,6 +42,7 @@
#include "virpidfile.h"
#include "virprocess.h"
#include "virbuffer.h"
+#include "virthread.h"
#define VIR_FROM_THIS VIR_FROM_NONE
@@ -80,15 +81,13 @@ struct _virCommand {
char **errbuf;
int infd;
+int inpipe;
int outfd;
int errfd;
int *outfdptr;
int *errfdptr;
-size_t inbufOffset;
-int inWatch;
-int outWatch;
-int errWatch;
+virThreadPtr asyncioThread;
bool handshake;
int handshakeWait[2];
@@ -784,8 +783,7 @@ virCommandNewArgs(const char *const*args)
cmd->handshakeNotify[0] = -1;
cmd->handshakeNotify[1] = -1;
-cmd->infd = cmd->outfd = cmd->errfd = -1;
-cmd->inWatch = cmd->outWatch = cmd->errWatch = -1;
+cmd->infd = cmd->inpipe = cmd->outfd = cmd->errfd = -1;
cmd->pid = -1;
virCommandAddArgSet(cmd, args);
@@ -1703,19 +1701,17 @@ virCommandToString(virCommandPtr cmd)
* Manage input and output to the child process.
*/
static int
-virCommandProcessIO(virCommandPtr cmd, int *inpipe)
+virCommandProcessIO(virCommandPtr cmd)
{
-int infd = -1, outfd = -1, errfd = -1;
+int outfd = -1, errfd = -1;
size_t inlen = 0, outlen = 0, errlen = 0;
size_t inoff = 0;
int ret = 0;
/* With an input buffer, feed data to child
* via pipe */
-if (cmd->inbuf) {
+if (cmd->inbuf)
inlen = strlen(cmd->inbuf);
-infd = *inpipe;
-}
/* With out/err buffer, the outfd/errfd have been filled with an
* FD for us. Guarantee an allocated string with partial results
@@ -1744,8 +1740,8 @@ virCommandProcessIO(virCommandPtr cmd, int *inpipe)
struct pollfd fds[3];
int nfds = 0;
-if (infd != -1) {
-fds[nfds].fd = infd;
+if (cmd->inpipe != -1) {
+fds[nfds].fd = cmd->inpipe;
fds[nfds].events = POLLOUT;
fds[nfds].revents = 0;
nfds++;
@@ -1817,21 +1813,19 @@ virCommandProcessIO(virCommandPtr cmd, int *inpipe)
}
if (fds[i].revents & (POLLOUT | POLLERR) &&
-fds[i].fd == infd) {
+fds[i].fd == cmd->inpipe) {
int done;
/* Coverity 5.3.0 can't see that we only get here if
* infd is in the set because it was non-negative. */
sa_assert(infd != -1);
-done = write(infd, cmd->inbuf + inoff,
+done = write(cmd->inpipe, cmd->inbuf + inoff,
inlen - inoff);
if (done < 0) {
if (errno == EPIPE) {
VIR_DEBUG("child closed stdin early, ignoring EPIPE "
- "on fd %d", infd);
-if (VIR_CLOSE(*inpipe) < 0)
-VIR_DEBUG("ignoring failed close on fd %d", infd);
-infd = -1;
+ "on fd %d", cmd->inpipe);
+VIR_FORCE_CLOSE(cmd->inpipe);
} else if (errno != EINTR && errno != EAGAIN) {
virReportSystemError(errno, "%s",
_("unable to write to child
input"));
@@ -1839,11 +1833,8 @@ virCommandProcessIO(virCommandPtr cmd, int *inpipe)
}
} else {
inoff += done;
-if (inoff == inlen) {
-if (VIR_CLOSE(*inpipe) < 0)
-VIR_DEBUG("ignoring failed close on fd %d", infd);
-infd = -1;
-}
+if (inoff == inlen)
+VIR_FORCE_CLOSE(cmd->inpipe);
}
}
}
@@ -1914,7 +1905,6 @@ virCommandRun(virCommandPtr cmd, int *exitstatus)
int ret = 0;
char *outbuf = NULL;
char *errbuf = NULL;
-int infd[2] = { -1, -1 };
struct stat st;
bool s
Re: [libvirt] [PATCH] virsh-snapshot: Fix xpath query to determine snapshot state
On 02/08/13 14:34, Jiri Denemark wrote: On Fri, Feb 08, 2013 at 14:26:28 +0100, Peter Krempa wrote: The query didn't match the external state correctly for offline internal snapshots. --- tools/virsh-snapshot.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) ACK, I wonder what did the old XPath actually do :-) Me neither :) and it was behaving really strange too. Pushed; Thanks. Peter -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] libvirt and Parallels Cloud Storage
В Thu, 7 Feb 2013 16:09:31 + "Daniel P. Berrange" пишет: > On Mon, Feb 04, 2013 at 08:13:20PM +0400, Alexander Gordeev wrote: > > Hi! > > > > I'd like to develop libvirt integration with Parallels Cloud > > Storage (Pstorage) which is a new distributed file system from > > Parallels designed specifically for storing VM images: > > http://www.parallels.com/products/pcs/cloud-storage/ > > Yay, yet another filesystem :-( Everyone seems to think they need to > write their own custom network/cluster/cloud FUSE filesystem these > days. Do you think Parallels would invest many man-years of development if we could just take existing solutions? :) It offers some unique combination of features: strong consistency, replication, high-availability. Also it's very fast. Let's compare: 1. CEPH uses BTRFS which is not considered stable yet. 2. Sheepdog has only synchronous writes and poor performance. 3. Glusterfs doesn't offer strong consistency. Strong consistency is required for real filesystems (NTFS, ext3/4, ...) because that's what HDDs offer. Please correct me if I'm wrong. I haven't tried these filesystems myself, this is just a summary from our internal wiki. Pstorage includes three components: metadata servers, chunk servers and clients. Metadata servers hold distributed metadata and synchronize it using PAXOS (this is how we offer strong consistency). Chunk servers store replicated pieces of VM images. The preferred method of access is FUSE client, but you are not limited to it. You can also access a Pstorage cluster directly using client library (FUSE client uses it too, of course). There is also an LD_PRELOAD library that intercepts syscalls like open, read, write, etc. I think it's the same approach as in Glusterfs. FUSE client is very fast when used with O_DIRECT + aio. Synchronous access is much slower. For example, on a single machine with one HDD async access gives almost the speed of HDD: 120-130MB/s. Sync access is about 2.5MB/s. AFAIK this cannot be fixed without some improvements in the kernel. We have some patches but we don't expect them to reach mainline soon. But we want to make it easy to use Pstorage with KVM/Xen and libvirt now. BTW I think user-configurable per-pool defaults can be useful for others as well. > > It's not a POSIX FS but there is a FUSE client for it that can be > > used to access and manipulate images. It's quite high speed but only > > when used with O_DIRECT + aio. I tried to setup several KVMs on top of > > a Pstorage mount using virt-manager. It worked good, but I had to: > > 1. tune cache and IO settings for every disk > > 2. disable space allocation by libvirt because it is using sync IO and > > is therefore slow > > > > I tried to find ways to solve the first issue and IMHO this can be > > done by adding a way to specify per-pool defaults for cache and IO > > settings. I didn't find any way for this in the current code and UI. > > I'd like to add a new storage backend also that will be a 'dir' backend > > with extra ability to manage Pstorage mount points and UI integration. > > I'd like to merge my work to the main tree when it's finished if > > possible. > > I don't think that putting cache/IO defaults in the XML is really > appropriate. There are too many different scenarios which want > different settings to be able to clearly identify one set of > perfect settings. I see this as primarily a documentation problem. > Someone needs to write something describing the diferrent settings > for each storage pool & what the pros/cons are for each. Downstream > app developers can then make decisions about suitable defaults for > their use cases Do you mean that I should patch virt-manager/oVirt for per-pool defaults instead? IMO it's a bad idea. Imagine that I create a pool on one machine and select some reasonable defaults for this pool in this instance of virt-manager. Then I create an image from another instance of virt-manager on a different machine which doesn't have these defaults and therefore will not set them. I think, this is not user-friendly. I already made a step-by-step guide about creating a VM on a pstorage pool. It's not quite easy. You should definitely avoid preallocating an image because it will be preallocated synchronously and also you have to set cache=none and io=native before starting VM for the installation or it will take hours. I think selecting some reasonable defaults is the right thing instead of forcing users to read documentation. -- Alexander -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] network_conf.c: Free xmlDoc after use
On 02/08/2013 09:26 AM, Michal Privoznik wrote: > The virNetworkObjUpdateParseFile() function was not freeing the xml > variable, leaving us with a memory leak. > --- > src/conf/network_conf.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c > index c93916d..3604ff7 100644 > --- a/src/conf/network_conf.c > +++ b/src/conf/network_conf.c > @@ -1895,6 +1895,7 @@ virNetworkObjUpdateParseFile(const char *filename, > ret = 0; > > cleanup: > +xmlFreeDoc(xml); > xmlXPathFreeContext(ctxt); > return ret; > } > ACK Neither Coverity or Valgrind picked this up! John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] network_conf.c: Free xmlDoc after use
On 08.02.2013 15:52, John Ferlan wrote: > On 02/08/2013 09:26 AM, Michal Privoznik wrote: >> The virNetworkObjUpdateParseFile() function was not freeing the xml >> variable, leaving us with a memory leak. >> --- >> src/conf/network_conf.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c >> index c93916d..3604ff7 100644 >> --- a/src/conf/network_conf.c >> +++ b/src/conf/network_conf.c >> @@ -1895,6 +1895,7 @@ virNetworkObjUpdateParseFile(const char *filename, >> ret = 0; >> >> cleanup: >> +xmlFreeDoc(xml); >> xmlXPathFreeContext(ctxt); >> return ret; >> } >> > ACK > > Neither Coverity or Valgrind picked this up! > > John > Thanks, pushed. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] libvirt and Parallels Cloud Storage
On 02/08/2013 06:47 AM, Alexander Gordeev wrote: В Thu, 7 Feb 2013 16:09:31 + "Daniel P. Berrange" пишет: On Mon, Feb 04, 2013 at 08:13:20PM +0400, Alexander Gordeev wrote: Hi! I'd like to develop libvirt integration with Parallels Cloud Storage (Pstorage) which is a new distributed file system from Parallels designed specifically for storing VM images: http://www.parallels.com/products/pcs/cloud-storage/ Yay, yet another filesystem :-( Everyone seems to think they need to write their own custom network/cluster/cloud FUSE filesystem these days. Do you think Parallels would invest many man-years of development if we could just take existing solutions? :) It offers some unique combination of features: strong consistency, replication, high-availability. Also it's very fast. Let's compare: These are all incorrect afaik: 1. CEPH uses BTRFS which is not considered stable yet. btrfs isn't required, xfs is actually recommended, and ext4 works fine too. btrfs just happens to be more efficient for some workloads. 2. Sheepdog has only synchronous writes and poor performance. sheepdog certainly has aio in their qemu driver, which is the main interface. It also isn't a distributed filesystem, it's just block storage. 3. Glusterfs doesn't offer strong consistency. Strong consistency is required for real filesystems (NTFS, ext3/4, ...) because that's what HDDs offer. I'm pretty sure glusterfs is supposed to be strongly consistent, although I haven't tested it myself. Josh -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 06/15] qemu: replace exec hook with virCommandSetUID/GID in qemuCaps*
On Thu, Feb 07, 2013 at 04:37:47PM -0500, Laine Stump wrote: > Setting the uid/gid of the child process was the only thing done by > the hook function in this case, and that can now be done more simply > with virCommandSetUID/GID. > --- > src/qemu/qemu_capabilities.c | 61 > +++- > 1 file changed, 15 insertions(+), 46 deletions(-) ACK, this is the kind of nice benefit we get from good apis :-) 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 07/15] qemu: replace exec hook with virCommandSetUID/GID in storage_backend
On Thu, Feb 07, 2013 at 04:37:48PM -0500, Laine Stump wrote: > --- > src/storage/storage_backend.c | 28 ++-- > 1 file changed, 6 insertions(+), 22 deletions(-) ACK 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 08/15] build: define SECDRIVER_LIBS in Makefile.am
On Thu, Feb 07, 2013 at 04:37:49PM -0500, Laine Stump wrote: > This makes it simpler to include the necessary system security driver > libraries for a particular system. For this patch, several existing > conditional sections from the Makfile were replaced; I'll later be > adding SECDRIVER_LIBS to libvirt_util_la_LIBADD, because vircommand.c > will be calling a function from $securitylib. > --- > src/Makefile.am | 32 +++- > 1 file changed, 11 insertions(+), 21 deletions(-) ACK 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 09/15] util: add security label setting to virCommand
On Thu, Feb 07, 2013 at 04:37:50PM -0500, Laine Stump wrote:
> virCommand gets the new API virCommandSetSecLabel(), which saves a
> copy of a null-terminated string in the virCommand. During
> virCommandRun, if the seclabel is non-NULL and we've been compiled
> with a security driver, the appropriate security library function is
> called to set the label for the child process. In the case of SELinux,
> setexeccon_raw() is called, and for AppArmor, aa_change_profile() is
> called.
>
> This functionality has been added so that users of virCommand can use
> the upcoming virSecurityManagerSetChildProcessLabel() prior to running
> a child process, rather than needing to setup a hook function to be
> called (and in turn call virSecurityManagerSetProcessLabel()) *during*
> the setup of the child process.
> ---
>
> +#if defined(WITH_SECDRIVER_SELINUX)
> +# include
> +#elif defined(WITH_SECDRIVER_APPARMOR)
> +# include
> +#endif
[snip]
> +/**
> + * virCommandSetSecLabel:
> + * @cmd: the command to modify
> + * @label: the label to use
> + *
> + * Saves a copy of @label to use when calling the appropriate security
> + * driver after the child process has been started. In the case of
> + * SELinux, this label will be sent to setexeccon_raw(), and in the
> + * case of AppArmor, it will be sent to aa_change_profile(). If
> + * neither of these is configured into libvirt, or if label is NULL,
> + * nothing will be done.
> + */
> +void
> +virCommandSetSecLabel(virCommandPtr cmd, const char *label)
> +{
> +if (!cmd || cmd->has_error)
> +return;
> +
> +VIR_FREE(cmd->seclabel);
> +if (label && !(cmd->seclabel = strdup(label)))
> +cmd->has_error = ENOMEM;
> +return;
> +}
It is technically possible to build libvirt with both apparmour and
selinux enabled, and choose between the impl with a libvirtd config.
This means we need to have separate methods for each in virCommand.
So I'd suggest a pair of methods
virCommandSetSELinuxLabel(...)
virCommandSetAppArmourProfile(...)
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 11/15] qemu: let virCommand set child process security labels/uid/gid
On Thu, Feb 07, 2013 at 04:37:52PM -0500, Laine Stump wrote: > The qemu driver had been calling virSecurityManagerSetProcessLabel() > from a "pre-exec hook" function that is run after the child is forked, > but before exec'ing qemu. This is problematic because the uid and gid > of the child are set by the security driver, but capabilities are > dropped by virCommand - such separation doesn't work; the two > operations must be done together or the capabilities do not transfer > properly to the child process. > > This patch switches to using virSecurityManagerSetChildProcessLabel(), > which is called prior to virCommandRun() (rather than being called > *during* virCommandrun() by the hook function), and doesn't set the > UID/GID/security label directly, but instead merely informs virCommand > what it should set them all to when the time is appropriate. > > This lets virCommand choose to do the uid/gid and caps dropping all at > the same time if it wants (it does *want* to, but isn't doing so yet; > that's for an upcoming patch). > --- > src/qemu/qemu_process.c | 12 +++- > 1 file changed, 7 insertions(+), 5 deletions(-) ACK 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 10/15] security: add new virSecurityManagerSetChildProcessLabel API
On Thu, Feb 07, 2013 at 04:37:51PM -0500, Laine Stump wrote: > The existing virSecurityManagerSetProcessLabel() API is designed so > that it must be called after forking the child process, but before > exec'ing the child. Due to the way the virCommand API works, that > means it needs to be put in a "hook" function that virCommand is told > to call out to at that time. > > Setting the child process label is a basic enough need when executing > any process that virCommand should have a method of doing that. But > virCommand must be told what label to set, and only the security > driver knows the answer to that question. > > The new virSecurityManagerSet*Child*ProcessLabel() API is the way to > transfer the knowledge about what label to set from the security > driver to the virCommand object. It is given a virCommandPtr, and each > security driver calls the appropriate virCommand* API to tell > virCommand what to do between fork and exec. > > 1) in the case of the DAC security driver, it calls > virCommandSetUID/GID() to set a uid and gid that must be set for the > child process. > > 2) for the SELinux and AppArmor security drivers, it calls > virCommandSetSecLabel() to save a copy of the char* that will be sent > to each driver's respective "SetProcessLabel" API *after forking > the child process*. This will need a tweak based on the change I suggested to the previous patch. > diff --git a/src/security/security_apparmor.c > b/src/security/security_apparmor.c > index bf795b0..4a81118 100644 > --- a/src/security/security_apparmor.c > +++ b/src/security/security_apparmor.c > @@ -1,7 +1,7 @@ > /* > * AppArmor security driver for libvirt > * > - * Copyright (C) 2011 Red Hat, Inc. > + * Copyright (C) 2011, 2013 Red Hat, Inc. You can turn this into a range '2011-2013' - there were plenty of Red Hat changes to this file throughout the last year(s). Likewise for all others. 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 2/4] net: support set public ip for forward mode nat
On Mon, Feb 04, 2013 at 10:45:24AM +0100, Natanael Copa wrote: > Support setting which public ip to use for NAT via attribute > address in subelement in : > > ... > > > Unless I'm mis-understanding, this is just identical to using a range, with the start + end addresses equal eg if so, then this is redundant - we should just use the syntax for everything, and not special case the scenario where start+end are equal. 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 1/4] util: refactor iptables command construction into multiple steps
On Mon, Feb 04, 2013 at 10:45:23AM +0100, Natanael Copa wrote: > Instead of creating an iptables command in one shot, do it in steps > so we can add conditional options like physdev and protocol. > > This removes code duplication while keeping existing behaviour. > > Signed-off-by: Natanael Copa > --- > This patch is unmodified since last time i sent it [1]. > > [1] https://www.redhat.com/archives/libvir-list/2012-November/msg00986.html > > src/util/viriptables.c | 130 > ++--- > 1 file changed, 58 insertions(+), 72 deletions(-) ACK, looks fine. 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 0/4] net: support set source address(es) and ports for NAT
On Mon, Feb 04, 2013 at 10:45:22AM +0100, Natanael Copa wrote: > This is a rework of the previous sent 'set public ip for nat' patches [1]. > > Changes v2: > - Use separate attributes for addresses and ports as suggested by Laine. > - support set port range without setting public ip > Broadly looks good to me. If you get rid of patch 2 which is just a special case of patch 3, then I'd ack 3 + 4. 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
[libvirt] [PATCHv2 1/5] S390: Documentation for CCW address type
The native bus for s390 I/O is called CCW (channel command word).
As QEMU has added basic support for the CCW bus, i.e. the
ability to assign CCW devnos (bus addresses) to devices.
Domains with the new machine type s390-ccw-virtio can use the
CCW bus. Currently QEMU will only allow to define virtio
devices on the CCW bus.
Here we add the new machine type and the new device address to the
schema definition and add a new paragraph to the domain XML
documentation.
Signed-off-by: Viktor Mihajlovski
---
V2 Changes
- replace single devno attribute with cssid, ssid, schid triple
in documentation section
- add new data ranges for cssid, ssid and schid to domain schema
docs/formatdomain.html.in | 14 +++
docs/schemas/domaincommon.rng | 52 +
2 files changed, 66 insertions(+)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index c01f564..ffcc33e 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -2018,6 +2018,20 @@
of the starting register). Since
0.9.9.
+ type='ccw'
+ s390 guests with a machine value of
+s390-ccw-virtio use the native CCW bus for I/O devices.
+CCW bus addresses have the following additional attributes:
+cssid (a hex value between 0 and 0xfe, inclusive),
+ssid (a value between 0 and 3, inclusive) and
+schid (a hex value between 0 and 0x, inclusive).
+Partially specified bus addresses are not allowed.
+If omitted, libvirt will assign a free bus address with
+cssid=0xfe and ssid=0. Virtio devices for s390 must have their
+cssid set to 0xfe in order to be recognized by the guest
+operating system.
+Since 1.0.3.
+
Controllers
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 049f232..63be4aa 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -401,7 +401,10 @@
+s390
s390-virtio
+s390-ccw
+s390-ccw-virtio
@@ -3037,6 +3040,19 @@
+
+
+
+
+
+
+
+
+
+
+
+
+
@@ -3469,6 +3485,12 @@
+
+
+ccw
+
+
+
@@ -3856,4 +3878,34 @@
+
+
+
+0x[0-9a-eA-E][0-9a-fA-F]?
+
+
+0x[fF][0-9a-eA-E]?
+
+
+0
+254
+
+
+
+
+
+ (0x)?[0-3]
+
+
+
+
+
+0x[0-9a-fA-F]{1,4}
+
+
+0
+65535
+
+
+
--
1.7.9.5
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv2 0/5] S390: Support for native CCW bus
Originally, QEMU did not implement a native I/O bus for s390. The initial implementation had a machine type 's390-virtio' featuring a fully paravirtualized I/O system with an artificial bus type 'virtio-s390'. This bus had a number of short-comings, like the need for a non-standard device discovery mechanism, a limited number of devices, limited hotplugging capabilites and the lack of persistent device addresses. To resolve these issues a new machine type 's390-ccw-virtio' has been recently added to QEMU which implementa the native s390 CCW I/O bus. Guests with arch='s390x' and machine='s390-ccw-virtio' can now be defined with up to 262144 virtio devices. This series adds the support for the s390-ccw-virtio machine type and the CCW bus to libvirt. As usual we start with the documentation/schema, the generic configuration stuff, continue with the QEMU driver including device hotplug and finally add qemu2xml testcases. V2 Changes - use an attribute triple cssid, ssid, schid instead of devno to better represent the libvirt CCW address structure - rebase to current upstream, mainly address qemuCapsXXX rename J.B. Joret (1): S390: Add hotplug support for s390 virtio devices Viktor Mihajlovski (4): S390: Documentation for CCW address type S390: domain_conf support for CCW S390: QEMU driver support for CCW addresses S390: Testcases for virtio-ccw machines docs/formatdomain.html.in | 14 + docs/schemas/domaincommon.rng | 52 src/conf/domain_conf.c | 107 +++- src/conf/domain_conf.h | 16 ++ src/libvirt_private.syms |1 + src/qemu/qemu_capabilities.c | 47 +++- src/qemu/qemu_capabilities.h |8 +- src/qemu/qemu_command.c| 269 +++- src/qemu/qemu_command.h|6 + src/qemu/qemu_domain.c |1 + src/qemu/qemu_domain.h |3 + src/qemu/qemu_driver.c | 26 +- src/qemu/qemu_hotplug.c| 147 +++ src/qemu/qemu_hotplug.h| 14 +- src/qemu/qemu_process.c| 12 +- .../qemuxml2argv-console-virtio-ccw.args | 10 + .../qemuxml2argv-console-virtio-ccw.xml| 27 ++ .../qemuxml2argv-console-virtio-s390.args |4 +- .../qemuxml2argv-console-virtio-s390.xml |2 +- .../qemuxml2argv-disk-virtio-ccw-many.args | 12 + .../qemuxml2argv-disk-virtio-ccw-many.xml | 40 +++ .../qemuxml2argv-disk-virtio-ccw.args |8 + .../qemuxml2argv-disk-virtio-ccw.xml | 30 +++ .../qemuxml2argv-net-virtio-ccw.args |8 + .../qemuxml2argv-net-virtio-ccw.xml| 30 +++ tests/qemuxml2argvtest.c | 11 +- tests/testutilsqemu.c |3 +- 27 files changed, 812 insertions(+), 96 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-console-virtio-ccw.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-console-virtio-ccw.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-ccw-many.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-ccw-many.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-ccw.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-ccw.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-virtio-ccw.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-virtio-ccw.xml -- 1.7.9.5 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv2 2/5] S390: domain_conf support for CCW
Add necessary handling code for the new s390 CCW address type to
virDomainDeviceInfo. Further, introduce memory management, XML
parsing, output formatting and range validation for the new
virDomainDeviceCCWAddress type.
Signed-off-by: Viktor Mihajlovski
---
V2 Changes
- adapted virDomainDeviceCCWAddressParseXML to handle the new
set of CCW address attributes
src/conf/domain_conf.c | 107 +-
src/conf/domain_conf.h | 16 +++
src/libvirt_private.syms |1 +
3 files changed, 123 insertions(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 5e16ddf..1c925f0 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -185,7 +185,8 @@ VIR_ENUM_IMPL(virDomainDeviceAddress,
VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST,
"ccid",
"usb",
"spapr-vio",
- "virtio-s390")
+ "virtio-s390",
+ "ccw")
VIR_ENUM_IMPL(virDomainDisk, VIR_DOMAIN_DISK_TYPE_LAST,
"block",
@@ -2124,6 +2125,13 @@ void virDomainObjListRemove(virDomainObjListPtr doms,
virObjectUnlock(doms);
}
+static int
+virDomainDeviceCCWAddressIsValid(virDomainDeviceCCWAddressPtr addr)
+{
+return addr->cssid <= VIR_DOMAIN_DEVICE_CCW_MAX_CSSID &&
+addr->ssid <= VIR_DOMAIN_DEVICE_CCW_MAX_SSID &&
+addr->schid <= VIR_DOMAIN_DEVICE_CCW_MAX_SCHID;
+}
int virDomainDeviceAddressIsValid(virDomainDeviceInfoPtr info,
int type)
@@ -2138,6 +2146,12 @@ int virDomainDeviceAddressIsValid(virDomainDeviceInfoPtr
info,
case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE:
return 1;
+case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390:
+return 1;
+
+case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW:
+return virDomainDeviceCCWAddressIsValid(&info->addr.ccw);
+
case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB:
return 1;
}
@@ -2219,6 +2233,19 @@ static int
virDomainDeviceInfoClearPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUS
return 0;
}
+static int
+virDomainDeviceInfoClearCCWAddress(virDomainDefPtr def ATTRIBUTE_UNUSED,
+ virDomainDeviceDefPtr device
ATTRIBUTE_UNUSED,
+ virDomainDeviceInfoPtr info,
+ void *opaque ATTRIBUTE_UNUSED)
+{
+if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) {
+memset(&info->addr, 0, sizeof(info->addr));
+info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE;
+}
+return 0;
+}
+
int virDomainDeviceInfoIterate(virDomainDefPtr def,
virDomainDeviceInfoCallback cb,
void *opaque)
@@ -2331,6 +2358,11 @@ void virDomainDefClearPCIAddresses(virDomainDefPtr def)
virDomainDeviceInfoIterate(def, virDomainDeviceInfoClearPCIAddress, NULL);
}
+void virDomainDefClearCCWAddresses(virDomainDefPtr def)
+{
+virDomainDeviceInfoIterate(def, virDomainDeviceInfoClearCCWAddress, NULL);
+}
+
void virDomainDefClearDeviceAliases(virDomainDefPtr def)
{
virDomainDeviceInfoIterate(def, virDomainDeviceInfoClearAlias, NULL);
@@ -2432,6 +2464,13 @@ virDomainDeviceInfoFormat(virBufferPtr buf,
virBufferAsprintf(buf, " reg='0x%llx'", info->addr.spaprvio.reg);
break;
+case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW:
+virBufferAsprintf(buf, " cssid='0x%x' ssid='0x%x' schid='0x%04x'",
+ info->addr.ccw.cssid,
+ info->addr.ccw.ssid,
+ info->addr.ccw.schid);
+break;
+
default:
virReportError(VIR_ERR_INTERNAL_ERROR,
_("unknown address type '%d'"), info->type);
@@ -2542,6 +2581,64 @@ cleanup:
}
static int
+virDomainDeviceCCWAddressParseXML(xmlNodePtr node,
+ virDomainDeviceCCWAddressPtr addr)
+{
+int ret = -1;
+char *cssid;
+char *ssid;
+char *schid;
+
+memset(addr, 0, sizeof(*addr));
+
+cssid = virXMLPropString(node, "cssid");
+ssid = virXMLPropString(node, "ssid");
+schid = virXMLPropString(node, "schid");
+
+if (cssid && ssid && schid) {
+if (cssid &&
+virStrToLong_ui(cssid, NULL, 0, &addr->cssid) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Cannot parse 'cssid' attribute"));
+goto cleanup;
+}
+if (ssid &&
+virStrToLong_ui(ssid, NULL, 0, &addr->ssid) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Cannot parse 'ssid' attribute"));
+goto cleanup;
+}
+if (schid &&
+virStrToLong_ui(schid, NULL, 0, &addr->schid) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Cannot parse 'schid' attribute"));
+goto cleanup;
+}
+
[libvirt] [PATCHv2 5/5] S390: Testcases for virtio-ccw machines
This adds and corrects testcases for virtio devices on s390 guests. Signed-off-by: Viktor Mihajlovski --- V2 Changes - adapt the testcase XML files to use the new attributes - use different variations for the values: hex, decimal, leading zeroes... .../qemuxml2argv-console-virtio-ccw.args | 10 + .../qemuxml2argv-console-virtio-ccw.xml| 27 + .../qemuxml2argv-console-virtio-s390.args |4 +- .../qemuxml2argv-console-virtio-s390.xml |2 +- .../qemuxml2argv-disk-virtio-ccw-many.args | 12 ++ .../qemuxml2argv-disk-virtio-ccw-many.xml | 40 .../qemuxml2argv-disk-virtio-ccw.args |8 .../qemuxml2argv-disk-virtio-ccw.xml | 30 +++ .../qemuxml2argv-net-virtio-ccw.args |8 .../qemuxml2argv-net-virtio-ccw.xml| 30 +++ tests/qemuxml2argvtest.c | 11 +- tests/testutilsqemu.c |3 +- 12 files changed, 180 insertions(+), 5 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-console-virtio-ccw.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-console-virtio-ccw.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-ccw-many.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-ccw-many.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-ccw.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-ccw.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-virtio-ccw.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-virtio-ccw.xml diff --git a/tests/qemuxml2argvdata/qemuxml2argv-console-virtio-ccw.args b/tests/qemuxml2argvdata/qemuxml2argv-console-virtio-ccw.args new file mode 100644 index 000..6660a30 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-console-virtio-ccw.args @@ -0,0 +1,10 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M \ +s390-ccw -m 214 -smp 1 -nographic -nodefconfig -nodefaults -chardev \ +socket,id=charmonitor,path=/tmp/test-monitor,server,nowait -mon \ +chardev=charmonitor,id=monitor,mode=readline -no-acpi \ +-device virtio-serial-ccw,id=virtio-serial0,devno=fe.0.0001 \ +-usb -drive file=/dev/HostVG/QEMUGuest1,if=none,id=drive-virtio-disk0 \ +-device virtio-blk-ccw,devno=fe.0.,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1 \ +-chardev pty,id=charconsole0 \ +-device virtconsole,chardev=charconsole0,id=console0 \ +-device virtio-balloon-ccw,id=balloon0,devno=fe.0.000a diff --git a/tests/qemuxml2argvdata/qemuxml2argv-console-virtio-ccw.xml b/tests/qemuxml2argvdata/qemuxml2argv-console-virtio-ccw.xml new file mode 100644 index 000..faaeeb8 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-console-virtio-ccw.xml @@ -0,0 +1,27 @@ + + QEMUGuest1 + c7a5fdbd-edaf-9455-926a-d65c16db1809 + 219100 + 219100 + +hvm + + + destroy + restart + destroy + +/usr/bin/qemu + + + + + + + + + + + + + diff --git a/tests/qemuxml2argvdata/qemuxml2argv-console-virtio-s390.args b/tests/qemuxml2argvdata/qemuxml2argv-console-virtio-s390.args index 3bd7817..bf7b180 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-console-virtio-s390.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-console-virtio-s390.args @@ -2,8 +2,8 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M \ s390-virtio -m 214 -smp 1 -nographic -nodefconfig -nodefaults -chardev \ socket,id=charmonitor,path=/tmp/test-monitor,server,nowait -mon \ chardev=charmonitor,id=monitor,mode=readline -no-acpi \ --boot c -device virtio-serial-s390,id=virtio-serial0 \ +-device virtio-serial-s390,id=virtio-serial0 \ -usb -drive file=/dev/HostVG/QEMUGuest1,if=none,id=drive-virtio-disk0 \ --device virtio-blk-s390,drive=drive-virtio-disk0,id=virtio-disk0 \ +-device virtio-blk-s390,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1 \ -chardev pty,id=charconsole0 \ -device virtconsole,chardev=charconsole0,id=console0 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-console-virtio-s390.xml b/tests/qemuxml2argvdata/qemuxml2argv-console-virtio-s390.xml index 5a4a9d4..221232d 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-console-virtio-s390.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-console-virtio-s390.xml @@ -5,7 +5,6 @@ 219100 hvm - destroy @@ -16,6 +15,7 @@ + diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-ccw-many.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-ccw-many.args new file mode 100644 index 000..ca0c157 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-ccw-many.args @@ -0,0 +1,12 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S \ +-M s390-ccw
[libvirt] [PATCHv2 3/5] S390: QEMU driver support for CCW addresses
This commit adds the QEMU driver support for CCW addresses. The
current QEMU only allows virtio devices to be attached to the
CCW bus. We named the new capability indicating that support
QEMU_CAPS_VIRTIO_CCW accordingly.
The fact that CCW devices can only be assigned to domains with a
machine type of s390-ccw-virtio requires a modification in the
capability handling approach.
First, the QEMU binary name alone will not suffice for capability
lookup, we need the machine type as well. For that purpose we
mangle the machine type into the cache lookup key.
The other thing is that the device support probing will
unfortunately always return both the old virtio-*-s390
and the new virtio-*-ccw devices. This makes it impossible to
choose the correct default device address type if the domain
definition XML doesn't contain explict addresses.
Therefore we apply a fix up in the cache lookup: depending
on the machine type we clear either the VIRTIO_S390 or the
VIRTIO_CCW capability.
The rest is more straight-forward, but since it's a new address
type, a bit bulky. The majority of the new functions deals
with CCW address generation and management.
Signed-off-by: Viktor Mihajlovski
---
V2 Changes
- rebase, mainly addressing the rename of qemuCapsXXX to virQEMUCapsXXX
src/qemu/qemu_capabilities.c | 47 ++--
src/qemu/qemu_capabilities.h |8 +-
src/qemu/qemu_command.c | 269 --
src/qemu/qemu_command.h |6 +
src/qemu/qemu_domain.c |1 +
src/qemu/qemu_domain.h |3 +
src/qemu/qemu_driver.c | 20 +++-
src/qemu/qemu_process.c | 12 +-
8 files changed, 333 insertions(+), 33 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 4efe052..011b88c 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -205,6 +205,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
"usb-serial", /* 125 */
"usb-net",
"add-fd",
+ "virtio-ccw",
);
@@ -684,7 +685,7 @@ virQEMUCapsInitGuest(virCapsPtr caps,
/* Ignore binary if extracting version info fails */
if (binary) {
-if (!(qemubinCaps = virQEMUCapsCacheLookup(cache, binary))) {
+if (!(qemubinCaps = virQEMUCapsCacheLookup(cache, binary, NULL))) {
virResetLastError();
VIR_FREE(binary);
}
@@ -707,7 +708,7 @@ virQEMUCapsInitGuest(virCapsPtr caps,
if (!kvmbin)
continue;
-if (!(kvmbinCaps = virQEMUCapsCacheLookup(cache, kvmbin))) {
+if (!(kvmbinCaps = virQEMUCapsCacheLookup(cache, kvmbin, NULL))) {
virResetLastError();
VIR_FREE(kvmbin);
continue;
@@ -1339,6 +1340,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = {
{ "usb-hub", QEMU_CAPS_USB_HUB },
{ "ich9-ahci", QEMU_CAPS_ICH9_AHCI },
{ "virtio-blk-s390", QEMU_CAPS_VIRTIO_S390 },
+{ "virtio-blk-ccw", QEMU_CAPS_VIRTIO_CCW },
{ "sclpconsole", QEMU_CAPS_SCLP_S390 },
{ "lsi53c895a", QEMU_CAPS_SCSI_LSI },
{ "virtio-scsi-pci", QEMU_CAPS_VIRTIO_SCSI_PCI },
@@ -1356,7 +1358,6 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = {
{ "usb-net", QEMU_CAPS_DEVICE_USB_NET},
};
-
static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioBlk[] = {
{ "multifunction", QEMU_CAPS_PCI_MULTIFUNCTION },
{ "bootindex", QEMU_CAPS_BOOTINDEX },
@@ -1411,6 +1412,10 @@ static struct virQEMUCapsObjectTypeProps
virQEMUCapsObjectProps[] = {
ARRAY_CARDINALITY(virQEMUCapsObjectPropsVirtioBlk) },
{ "virtio-net-pci", virQEMUCapsObjectPropsVirtioNet,
ARRAY_CARDINALITY(virQEMUCapsObjectPropsVirtioNet) },
+{ "virtio-blk-ccw", virQEMUCapsObjectPropsVirtioBlk,
+ ARRAY_CARDINALITY(virQEMUCapsObjectPropsVirtioBlk) },
+{ "virtio-net-ccw", virQEMUCapsObjectPropsVirtioNet,
+ ARRAY_CARDINALITY(virQEMUCapsObjectPropsVirtioNet) },
{ "virtio-blk-s390", virQEMUCapsObjectPropsVirtioBlk,
ARRAY_CARDINALITY(virQEMUCapsObjectPropsVirtioBlk) },
{ "virtio-net-s390", virQEMUCapsObjectPropsVirtioNet,
@@ -1662,7 +1667,7 @@ int virQEMUCapsGetDefaultVersion(virCapsPtr caps,
return -1;
}
-if (!(qemucaps = virQEMUCapsCacheLookup(capsCache, binary)))
+if (!(qemucaps = virQEMUCapsCacheLookup(capsCache, binary, NULL)))
return -1;
*version = virQEMUCapsGetVersion(qemucaps);
@@ -2612,16 +2617,24 @@ error:
virQEMUCapsPtr
-virQEMUCapsCacheLookup(virQEMUCapsCachePtr cache, const char *binary)
+virQEMUCapsCacheLookup(virQEMUCapsCachePtr cache, const char *binary,
+const char *machine)
{
virQEMUCapsPtr ret = NULL;
+char * cachekey;
+
+if (virAsprintf(&cachekey, "%s:%s", binary, machine ? machine : "") < 0) {
+virReportOOMError();
+goto error;
+}
+
virMutexLock(&cache->lock);
-ret = virHashLooku
[libvirt] [PATCHv2 4/5] S390: Add hotplug support for s390 virtio devices
From: "J.B. Joret"
We didn't yet expose the virtio device attach and detach functionality
for s390 domains as the device hotplug was very limited with the old
virtio-s390 bus. With the CCW bus there's full hotplug support for
virtio devices in QEMU, so we are adding this to libvirt too.
Since the virtio hotplug isn't limited to PCI anymore, we change the
function names from xxxPCIyyy to xxxVirtioyyy, where we handle all
three virtio bus types.
Signed-off-by: J.B. Joret
Signed-off-by: Viktor Mihajlovski
---
V2 Changes
- rebase, mainly addressing the rename of qemuCapsXXX to virQEMUCapsXXX
src/qemu/qemu_driver.c |6 +-
src/qemu/qemu_hotplug.c | 147 ---
src/qemu/qemu_hotplug.h | 14 ++---
3 files changed, 110 insertions(+), 57 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 6e73558..62c95d6 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5865,7 +5865,7 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
ret = qemuDomainAttachUsbMassstorageDevice(conn, driver, vm,
disk);
} else if (disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) {
-ret = qemuDomainAttachPciDiskDevice(conn, driver, vm, disk);
+ret = qemuDomainAttachVirtioDiskDevice(conn, driver, vm, disk);
} else if (disk->bus == VIR_DOMAIN_DISK_BUS_SCSI) {
ret = qemuDomainAttachSCSIDisk(conn, driver, vm, disk);
} else {
@@ -5998,7 +5998,7 @@ qemuDomainDetachDeviceDiskLive(virQEMUDriverPtr driver,
case VIR_DOMAIN_DISK_DEVICE_DISK:
case VIR_DOMAIN_DISK_DEVICE_LUN:
if (disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO)
-ret = qemuDomainDetachPciDiskDevice(driver, vm, dev);
+ret = qemuDomainDetachVirtioDiskDevice(driver, vm, dev);
else if (disk->bus == VIR_DOMAIN_DISK_BUS_SCSI ||
disk->bus == VIR_DOMAIN_DISK_BUS_USB)
ret = qemuDomainDetachDiskDevice(driver, vm, dev);
@@ -6009,7 +6009,7 @@ qemuDomainDetachDeviceDiskLive(virQEMUDriverPtr driver,
default:
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("disk device type '%s' cannot be detached"),
- virDomainDiskDeviceTypeToString(disk->type));
+ virDomainDiskDeviceTypeToString(disk->device));
break;
}
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 4504f0b..4cea71f 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -224,11 +224,10 @@ cleanup:
return ret;
}
-
-int qemuDomainAttachPciDiskDevice(virConnectPtr conn,
- virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- virDomainDiskDefPtr disk)
+int qemuDomainAttachVirtioDiskDevice(virConnectPtr conn,
+ virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainDiskDefPtr disk)
{
int i, ret = -1;
const char* type = virDomainDiskBusTypeToString(disk->bus);
@@ -238,6 +237,14 @@ int qemuDomainAttachPciDiskDevice(virConnectPtr conn,
bool releaseaddr = false;
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+if (!disk->info.type) {
+if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_CCW))
+disk->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW;
+else if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_S390))
+disk->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390;
+else disk->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
+}
+
for (i = 0 ; i < vm->def->ndisks ; i++) {
if (STREQ(vm->def->disks[i]->dst, disk->dst)) {
virReportError(VIR_ERR_OPERATION_FAILED,
@@ -258,8 +265,14 @@ int qemuDomainAttachPciDiskDevice(virConnectPtr conn,
}
if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
-if (qemuDomainPCIAddressEnsureAddr(priv->pciaddrs, &disk->info) < 0)
-goto error;
+if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) {
+if (qemuDomainCCWAddressAssign(&disk->info, priv->ccwaddrs,
+ !disk->info.addr.ccw.assigned) < 0)
+goto error;
+} else if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {
+if (qemuDomainPCIAddressEnsureAddr(priv->pciaddrs, &disk->info) <
0)
+goto error;
+}
releaseaddr = true;
if (qemuAssignDeviceDiskAlias(vm->def, disk, priv->qemuCaps) < 0)
goto error;
@@ -294,16 +307,14 @@ int qemuDomainAttachPciDiskDevice(virConnectPtr conn,
}
}
}
-} else {
+} else if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI){
virDevicePCIAddress gue
Re: [libvirt] [PATCH] network_conf.c: Free xmlDoc after use
On 02/08/2013 07:52 AM, John Ferlan wrote: > On 02/08/2013 09:26 AM, Michal Privoznik wrote: >> The virNetworkObjUpdateParseFile() function was not freeing the xml >> variable, leaving us with a memory leak. >> --- >> src/conf/network_conf.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c >> index c93916d..3604ff7 100644 >> --- a/src/conf/network_conf.c >> +++ b/src/conf/network_conf.c >> @@ -1895,6 +1895,7 @@ virNetworkObjUpdateParseFile(const char *filename, >> ret = 0; >> >> cleanup: >> +xmlFreeDoc(xml); >> xmlXPathFreeContext(ctxt); >> return ret; >> } >> > ACK > > Neither Coverity or Valgrind picked this up! Hmm - on the Coverity side, that probably means doesn't know how to model virXMLParse as an allocating function, or its was being filtered out by hueristics. I wonder if Coverity is missing any other resource leaks, or if we could add annotations to allocating functions to make Coverity's job easier. On the Valgrind side, that makes sense - valgrind only detects leaks according to your actual execution usage; if you never pass libvirt input that triggers this code path, valgrind can't complain about it. I'm sure that it would be possible to write a test case that triggers this code and would make valgrind complain; but don't know how easy or hard it would be to get this extra coverage. Our tests definitely don't have 100% code coverage yet, and probably never will (you hit a point of diminishing returns, where it gets harder to trigger certain code paths). -- 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 07/15] qemu: replace exec hook with virCommandSetUID/GID in storage_backend
On 02/07/2013 02:37 PM, Laine Stump wrote: > --- > src/storage/storage_backend.c | 28 ++-- > 1 file changed, 6 insertions(+), 22 deletions(-) > > @@ -576,7 +558,9 @@ static int > virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, > } > } > > -data.skip = true; > +/* don't change uid/gid if we retry */ > +virCommandSetUID(cmd, 0); > +virCommandSetGID(cmd, 0); Hmm, so you are reusing an existing virCommand, but want to change it to no longer attempt uid/gid change (that is, inherit the uid/gid of the current libvirtd). If you refactor things in earlier patches to use -1 as the no-op, and allow an attempt to change to id 0, then this needs alteration to -1. And per the man page of setfsuid, there really are reasons why one would attempt to change uid to 0, even when already executing as uid 0 - it forces Linux to resync the fsuid back to 0. True, not much code plays with fsuid, and it is rare to have a program where fsuid differs from uid, but I'd rather our wrapper lets us expose full kernel/glibc rules on id setting, than to accidentally short-circuit away something that has important side-effects in odd corner cases. -- 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: support vhost-net for generic ethernet devices
>From qemu's point of view these are still just tap devices, so there's
no reason they shouldn't work with vhost-net; as a matter of fact,
Raja Sivaramakrishnan verified on libvir-list
that at least the qemu_command.c part of this patch works:
https://www.redhat.com/archives/libvir-list/2012-December/msg01314.html
(the hotplug case is extrapolation on my part).
---
src/qemu/qemu_command.c | 1 +
src/qemu/qemu_hotplug.c | 3 +++
2 files changed, 4 insertions(+)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 33f2ecd..6c28123 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -6159,6 +6159,7 @@ qemuBuildCommandLine(virConnectPtr conn,
if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
actualType == VIR_DOMAIN_NET_TYPE_BRIDGE ||
+actualType == VIR_DOMAIN_NET_TYPE_ETHERNET ||
actualType == VIR_DOMAIN_NET_TYPE_DIRECT) {
/* Attempt to use vhost-net mode for these types of
network device */
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 4504f0b..0c28a6a 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -754,6 +754,9 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
iface_connected = true;
if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, &vhostfd) < 0)
goto cleanup;
+} else if (actualType == VIR_DOMAIN_NET_TYPE_ETHERNET) {
+if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, &vhostfd) < 0)
+goto cleanup;
}
if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NET_NAME) ||
--
1.7.11.7
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: support vhost-net for generic ethernet devices
On 02/08/2013 10:58 AM, Laine Stump wrote: >>From qemu's point of view these are still just tap devices, so there's > no reason they shouldn't work with vhost-net; as a matter of fact, > Raja Sivaramakrishnan verified on libvir-list > that at least the qemu_command.c part of this patch works: > > https://www.redhat.com/archives/libvir-list/2012-December/msg01314.html > > (the hotplug case is extrapolation on my part). > --- > src/qemu/qemu_command.c | 1 + > src/qemu/qemu_hotplug.c | 3 +++ > 2 files changed, 4 insertions(+) ACK. -- 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] qemu: support vhost-net for generic ethernet devices
On 02/08/2013 01:04 PM, Eric Blake wrote: > On 02/08/2013 10:58 AM, Laine Stump wrote: >> >From qemu's point of view these are still just tap devices, so there's >> no reason they shouldn't work with vhost-net; as a matter of fact, >> Raja Sivaramakrishnan verified on libvir-list >> that at least the qemu_command.c part of this patch works: >> >> https://www.redhat.com/archives/libvir-list/2012-December/msg01314.html >> >> (the hotplug case is extrapolation on my part). >> --- >> src/qemu/qemu_command.c | 1 + >> src/qemu/qemu_hotplug.c | 3 +++ >> 2 files changed, 4 insertions(+) > ACK. > Pushed, Thanks! -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 09/15] util: add security label setting to virCommand
On 02/08/2013 11:23 AM, Daniel P. Berrange wrote:
> On Thu, Feb 07, 2013 at 04:37:50PM -0500, Laine Stump wrote:
>> virCommand gets the new API virCommandSetSecLabel(), which saves a
>> copy of a null-terminated string in the virCommand. During
>> virCommandRun, if the seclabel is non-NULL and we've been compiled
>> with a security driver, the appropriate security library function is
>> called to set the label for the child process. In the case of SELinux,
>> setexeccon_raw() is called, and for AppArmor, aa_change_profile() is
>> called.
>>
>> This functionality has been added so that users of virCommand can use
>> the upcoming virSecurityManagerSetChildProcessLabel() prior to running
>> a child process, rather than needing to setup a hook function to be
>> called (and in turn call virSecurityManagerSetProcessLabel()) *during*
>> the setup of the child process.
>> ---
>>
>> +#if defined(WITH_SECDRIVER_SELINUX)
>> +# include
>> +#elif defined(WITH_SECDRIVER_APPARMOR)
>> +# include
>> +#endif
> [snip]
>
>> +/**
>> + * virCommandSetSecLabel:
>> + * @cmd: the command to modify
>> + * @label: the label to use
>> + *
>> + * Saves a copy of @label to use when calling the appropriate security
>> + * driver after the child process has been started. In the case of
>> + * SELinux, this label will be sent to setexeccon_raw(), and in the
>> + * case of AppArmor, it will be sent to aa_change_profile(). If
>> + * neither of these is configured into libvirt, or if label is NULL,
>> + * nothing will be done.
>> + */
>> +void
>> +virCommandSetSecLabel(virCommandPtr cmd, const char *label)
>> +{
>> +if (!cmd || cmd->has_error)
>> +return;
>> +
>> +VIR_FREE(cmd->seclabel);
>> +if (label && !(cmd->seclabel = strdup(label)))
>> +cmd->has_error = ENOMEM;
>> +return;
>> +}
> It is technically possible to build libvirt with both apparmour and
> selinux enabled, and choose between the impl with a libvirtd config.
Until I broke it in the previous patch :-). I'll fix that as well.
>
> This means we need to have separate methods for each in virCommand.
> So I'd suggest a pair of methods
>
> virCommandSetSELinuxLabel(...)
> virCommandSetAppArmourProfile(...)
>
>
> Daniel
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 1/2] virDomainSaveXML: Reject domains which name contain '/'
John Ferlan wrote:
> On 02/07/2013 08:27 AM, Christophe Fergeau wrote:
>
>> Similarly to 790f912b4 which rejects snapshots names containing,
>> this commit changes virDomainSaveXML to reject domains with a '/'
>> in their name. The domain name is used as a filename, so this
>> leads to unexpected results when used in combination with '..'
>> ---
>> src/conf/domain_conf.c | 7 +++
>> 1 file changed, 7 insertions(+)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 85a798d..13f4bc0 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -14728,6 +14728,13 @@ int virDomainSaveXML(const char *configDir,
>> char *configFile = NULL;
>> int ret = -1;
>>
>> +if (strchr(def->name, '/')) {
>> +virReportError(VIR_ERR_XML_DETAIL,
>> + _("invalid domain name '%s': name can't contain
>> '/'"),
>> + def->name);
>> +goto cleanup;
>> +}
>> +
>> if ((configFile = virDomainConfigFile(configDir, def->name)) == NULL)
>> goto cleanup;
>>
>>
>>
> Seems this should be in a more "general" location. Would the same rules apply
> to
> other objects (networks, storage, etc.)? What other characters should be
> avoided?
> Having a comma, semi-colon, colon, etc. could have interesting results.
Yeah, comma is an interesting one for qemu since that delimits option
subarguments. E.g. trying to start a qemu instance with name 'foo,bar'
results in
$ virsh start "foo,bar"
error: Failed to start domain foo,bar
error: internal error process exited while connecting to monitor:
Unknown subargument bar to -name
Regards,
Jim
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 12/15] util: drop capabilities immediately after changing uid/gid of child
On 02/07/2013 02:37 PM, Laine Stump wrote: > This is an interim measure to make sure everything still works in this > order. The next step will be to perform capabilities drop and > setuid/gid as a single operation (which is the only way to keep any > capabilities when switching to a non-root uid). > --- > src/util/vircommand.c | 12 ++-- > 1 file changed, 6 insertions(+), 6 deletions(-) ACK. -- 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 1/2] virDomainSaveXML: Reject domains which name contain '/'
On 02/08/2013 11:44 AM, Jim Fehlig wrote: >> Seems this should be in a more "general" location. Would the same rules >> apply to >> other objects (networks, storage, etc.)? What other characters should be >> avoided? >> Having a comma, semi-colon, colon, etc. could have interesting results. > > Yeah, comma is an interesting one for qemu since that delimits option > subarguments. E.g. trying to start a qemu instance with name 'foo,bar' > results in > > $ virsh start "foo,bar" > error: Failed to start domain foo,bar > error: internal error process exited while connecting to monitor: > Unknown subargument bar to -name That's an independent bug - we already have a function for properly quoting commas when passing to qemu; if we would use that function properly, the command line would be -name foo,,bar, at which point qemu would be using 'foo,bar' internally anywhere the name is needed. -- 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 13/15] util: virSetUIDGIDWithCaps - change uid while keeping caps
On 02/07/2013 02:37 PM, Laine Stump wrote:
> Normally when a process' uid is changed to non-0, all the capabilities
> bits are cleared, even those explicitly set with calls to
> capng_update()/capng_apply() made immediately before setuid. And
> *after* the process' uid has been changed, it no longer has the
> necessary privileges to add capabilities back to the process.
>
> In order to set a non-0 uid while still maintaining any capabilities
> bits, it is necessary to either call capng_change_id() (which
> unfortunately doesn't not currently call initgroups to setup auxiliary
s/doesn't not/doesn't/
> group membership), or to perform the small amount of calisthenics
> contained in the new utility function virSetUIDGIDWithCaps().
>
>
> Note that, due to the way that effective capabilities are computed (at
> time of execve) for a process that has uid != 0, the *file*
> capabilities of the binary being executed must also have the desired
> capabilities bit(s) set (see "man 7 capabilities"). This can be done
> with the "filecap" command
> (e.g. "filecap /usr/bin/qemu-kvm sys_rawio").
Sheesh - does that mean that we have to fix up qemu packaging for Fedora
to include that capability by default?
>
> +#if WITH_CAPNG
> +/* Set the real and effective uid and gid to the given values, while
> + * maintaining the capabilities indicated by bits in @capBits. return
> + * 0 on success, -1 on failure (the original system error remains in
> + * errno).
> + */
> +int
> +virSetUIDGIDWithCaps(uid_t uid, gid_t gid, unsigned long long capBits)
> +{
> +int ii, capng_ret, ret = -1;
> +bool need_setgid = false, need_setuid = false;
> +bool need_prctl = false, need_setpcap = false;
> +
> +/* First drop all caps except those in capBits + the extra ones we
> + * need to change uid/gid and change the capabilities bounding
> + * set.
> + */
> +
> +capng_clear(CAPNG_SELECT_BOTH);
> +
> +for (ii = 0; ii <= CAP_LAST_CAP; ii++) {
> +if (capBits & (1ULL << ii)) {
> +capng_update(CAPNG_ADD,
> + CAPNG_EFFECTIVE|CAPNG_INHERITABLE|
> + CAPNG_PERMITTED|CAPNG_BOUNDING_SET,
> + ii);
> +}
> +}
> +
> +if (gid != -1 && gid > 0 && !capng_have_capability(CAPNG_EFFECTIVE,
> CAP_SETGID)) {
Another site that needs casting (gid != (gid_t) -1); and do we really
want to short-circuit the gid>0 case?
> +need_setgid = true;
> +capng_update(CAPNG_ADD, CAPNG_EFFECTIVE|CAPNG_PERMITTED, CAP_SETGID);
> +}
> +if (uid != -1 && uid > 0 && !capng_have_capability(CAPNG_EFFECTIVE,
> CAP_SETUID)) {
> +need_setuid = true;
> +capng_update(CAPNG_ADD, CAPNG_EFFECTIVE|CAPNG_PERMITTED, CAP_SETUID);
> +}
> +# ifdef PR_CAPBSET_DROP
> +// If newer kernel, we need also need setpcap to change the bounding set
Prefer /**/ comment style, please.
> +if ((capBits || need_setgid || need_setuid) &&
> +!capng_have_capability(CAPNG_EFFECTIVE, CAP_SETPCAP)) {
> +need_setpcap = true;
> +}
> +if (need_setpcap)
> +capng_update(CAPNG_ADD,CAPNG_EFFECTIVE|CAPNG_PERMITTED,
> CAP_SETPCAP);
Odd spacing.
> +# endif
> +
> +need_prctl = capBits || need_setgid || need_setuid || need_setpcap;
> +
> +// Tell system we want to keep caps across uid change
More comment style.
> +if (need_prctl && prctl(PR_SET_KEEPCAPS, 1, 0, 0, 0)) {
> +virReportSystemError(errno, "%s",
> + _("prctl failed to set KEEPCAPS"));
> +goto cleanup;
> +}
> +
> +// Change to the temp capabilities
And again.
> +if ((capng_ret = capng_apply(CAPNG_SELECT_BOTH)) < 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("cannot apply process capabilities %d"), capng_ret);
> +goto cleanup;
> +}
> +
> +/* Set UID/GID */
> +if (virSetUIDGID(uid, gid) < 0)
> +goto cleanup;
> +
> +// Tell it we are done keeping capabilities
and again
> +}
> +
> +#else
> +/*
> + * On platforms without libcapng, the capabilities setting is treated
> + * as a NOP.
> + */
> +
> +int
> +virSetUIDGIDWithCaps(uid_t uid, gid_t gid, unsigned long long capBits)
Need ATTRIBUTE_UNUSED on capBits.
> +{
> +return virSetUIDGID(uid, gid);
> +}
> +#endif
> +
>
> #if defined HAVE_MNTENT_H && defined HAVE_GETMNTENT_R
> /* search /proc/mounts for mount point of *type; return pointer to
> diff --git a/src/util/virutil.h b/src/util/virutil.h
> index 4201aa1..2dc3403 100644
> --- a/src/util/virutil.h
> +++ b/src/util/virutil.h
> @@ -54,6 +54,7 @@ int virPipeReadUntilEOF(int outfd, int errfd,
> char **outbuf, char **errbuf);
>
> int virSetUIDGID(uid_t uid, gid_t gid);
> +int virSetUIDGIDWithCaps(uid_t uid, gid_t gid, unsigned long long capBits);
>
> int virFileReadLimFD(int fd, int maxlen, char **buf) ATTRIBUTE_RETURN_CHECK;
>
>
--
Eric Blake eblake redhat com
Re: [libvirt] [PATCH 14/15] util: maintain caps when running command with uid != 0
On 02/07/2013 02:37 PM, Laine Stump wrote:
> virCommand was previously calling virSetUIDGID() to change the uid and
> gid of the child process, then separately calling
> virSetCapabilities(). This did not work if the desired uid was != 0,
> since a setuid to anything other than 0 normally clears all
> capabilities bits.
>
> The solution is to use the new virSetUIDGIDWithCaps(), sending it the
> uid, gid, and capabilities bits. This will get the new process setup
> properly.
>
> Since the static functions virSetCapabilities() and
> virClearCapabilities are no longer called, they have been removed.
>
> NOTE: When combined with "filecap $path-to-qemu sys_rawio", this patch
> will make CAP_SYS_RAWIO (which is required for passthrough of generic
> scsi commands to a guest - see commits e8daeeb, 177db08, 397e6a7, and
> 74e0349) be retained by qemu when necessary. Apparently that
> capability has been broken for non-root qemu every since it was
s/every/ever/
> originally added.
> ---
> src/util/vircommand.c | 76
> ++-
> 1 file changed, 8 insertions(+), 68 deletions(-)
ACK.
> -# else
> -static int virClearCapabilities(void)
> -{
> -//VIR_WARN("libcap-ng support not compiled in, unable to clear "
> -// "capabilities");
Odd that we had commented this out previously. Should patch 13/15 log
any warnings when we are not preserving/clearing capabilities, rather
than silently ignoring the capability request?
>
> -if (cmd->uid > 0 || cmd->gid > 0) {
> -VIR_DEBUG("Setting child uid:gid to %u:%u", cmd->uid, cmd->gid);
> -if (virSetUIDGID(cmd->uid, cmd->gid) < 0)
> +/* The steps above may need todo something privileged, so we delay
As long as you are touching this comment, s/todo/to do/ (but you've
moved it at least twice in this series, so it depends on how much churn
you want on when you finally fix it).
--
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 15/15] qemu: set CAP_COMPROMISE_KERNEL so that pci passthrough works
On 02/07/2013 02:37 PM, Laine Stump wrote: > Any system with CAP_COMPROMISE_KERNEL available in the kernel was not > able to perform PCI passthrough device assignment without 1) running > qemu as root *and* 2) setting "clear_emulator_capabilities=0" in > /etc/libvirt/qemu.conf. > > This patch is the final piece to make pci passthrough once again work > properly with a non-root qemu. It sets CAP_COMPROMISE_KERNEL; now that > virCommand is properly setup to honor that request for non-root child > processes, it will actually do some good. > > It is still necessary to set the file capability for the qemu binary, > however (see the rules for determining effective caps of a process > running as non-root in "man 7 capabilities"). This can be done with: > > filecap $path-to-qemu-binary compromise_kernel Sounds like something that should be done by default at least for the Fedora packaging of qemu - that is, if the kernel folks don't honor our request to make CAP_COMPROMISE_KERNEL needed only on open() rather than all read()/write(). We may not need this patch, if the kernel folks are sensible. -- 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] Thoughts on adding a domain type of jvm
I'm working with a java vm vendor that is developing a multi-tenancy jvm and is trying to leverage the libvirt framework for the domain configuration. Right now the java language binding leverages the libvirt c library for parsing the domain.xml file. What are the thoughts on adding a 'jvm' domain type? I believe there would need to be a jvm specific section in a separate namespace similar to the qemucmdline section. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 15/15] qemu: set CAP_COMPROMISE_KERNEL so that pci passthrough works
On Fri, Feb 08, 2013 at 12:07:16PM -0700, Eric Blake wrote: > On 02/07/2013 02:37 PM, Laine Stump wrote: > > Any system with CAP_COMPROMISE_KERNEL available in the kernel was not > > able to perform PCI passthrough device assignment without 1) running > > qemu as root *and* 2) setting "clear_emulator_capabilities=0" in > > /etc/libvirt/qemu.conf. > > > > This patch is the final piece to make pci passthrough once again work > > properly with a non-root qemu. It sets CAP_COMPROMISE_KERNEL; now that > > virCommand is properly setup to honor that request for non-root child > > processes, it will actually do some good. > > > > It is still necessary to set the file capability for the qemu binary, > > however (see the rules for determining effective caps of a process > > running as non-root in "man 7 capabilities"). This can be done with: > > > > filecap $path-to-qemu-binary compromise_kernel > > Sounds like something that should be done by default at least for the > Fedora packaging of qemu - that is, if the kernel folks don't honor our > request to make CAP_COMPROMISE_KERNEL needed only on open() rather than > all read()/write(). > > We may not need this patch, if the kernel folks are sensible. Yes, I want to push this back onto the kernel developers. IMHO this is a userspace ABI change they've made here. The secureboot stuff should be a complete no-op if the kernel is not booted in secureboot mode, but the current kernel patch does not satisfy that. I don't think it should be libvirt or KVM's job to fix this kernel breakage. 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 1/4] qemu: Add checking in helpers for sgio setting
On 02/08/2013 08:07 AM, Osier Yang wrote:
> This moves the checking into the helpers, to avoid the
> callers missing the checking.
> ---
> src/qemu/qemu_conf.c| 20
> src/qemu/qemu_conf.h|4 ++--
> src/qemu/qemu_driver.c | 18 +++---
> src/qemu/qemu_process.c | 21 -
> 4 files changed, 37 insertions(+), 26 deletions(-)
>
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index 17f7d45..69ba710 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -726,12 +726,20 @@ qemuGetSharedDiskKey(const char *disk_path)
> */
> int
> qemuAddSharedDisk(virHashTablePtr sharedDisks,
> - const char *disk_path)
> + virDomainDiskDefPtr disk)
> {
> size_t *ref = NULL;
> char *key = NULL;
>
> -if (!(key = qemuGetSharedDiskKey(disk_path)))
> +/* Currently the only conflicts we have to care about
> + * for the shared disk is "sgio" setting, which is only
> + * valid for block disk.
> + */
> +if (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK ||
> +!disk->shared || !disk->src)
> +return 0;
> +
> +if (!(key = qemuGetSharedDiskKey(disk->src)))
> return -1;
>
> if ((ref = virHashLookup(sharedDisks, key))) {
> @@ -755,12 +763,16 @@ qemuAddSharedDisk(virHashTablePtr sharedDisks,
> */
> int
> qemuRemoveSharedDisk(virHashTablePtr sharedDisks,
> - const char *disk_path)
> + virDomainDiskDefPtr disk)
> {
> size_t *ref = NULL;
> char *key = NULL;
>
> -if (!(key = qemuGetSharedDiskKey(disk_path)))
> +if (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK ||
> +!disk->shared || !disk->src)
> +return 0;
> +
> +if (!(key = qemuGetSharedDiskKey(disk->src)))
> return -1;
>
> if (!(ref = virHashLookup(sharedDisks, key))) {
> diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
> index 60c4109..8e84bcf 100644
> --- a/src/qemu/qemu_conf.h
> +++ b/src/qemu/qemu_conf.h
> @@ -270,11 +270,11 @@ void qemuDriverCloseCallbackRunAll(virQEMUDriverPtr
> driver,
> virConnectPtr conn);
>
> int qemuAddSharedDisk(virHashTablePtr sharedDisks,
> - const char *disk_path)
> + virDomainDiskDefPtr disk)
> ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
>
> int qemuRemoveSharedDisk(virHashTablePtr sharedDisks,
> - const char *disk_path)
> + virDomainDiskDefPtr disk)
> ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
> char * qemuGetSharedDiskKey(const char *disk_path)
> ATTRIBUTE_NONNULL(1);
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 979a027..043efd3 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -5855,11 +5855,9 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
> }
>
> if (ret == 0) {
> -if (disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK && disk->shared) {
> -if (qemuAddSharedDisk(driver->sharedDisks, disk->src) < 0)
> -VIR_WARN("Failed to add disk '%s' to shared disk table",
> - disk->src);
> -}
> +if (qemuAddSharedDisk(driver->sharedDisks, disk) < 0)
> +VIR_WARN("Failed to add disk '%s' to shared disk table",
> + disk->src);
>
> if (qemuSetUnprivSGIO(disk) < 0)
> VIR_WARN("Failed to set unpriv_sgio of disk '%s'", disk->src);
Does there need to be a NULLSTR(disk->src)? Doesn't seem so, but just
checking. I only note this because the qemuAddSharedDisk() has a
!disk->src check prior to returning zero.
> @@ -5980,12 +5978,10 @@ qemuDomainDetachDeviceDiskLive(virQEMUDriverPtr
> driver,
> break;
> }
>
> -if (ret == 0 &&
> -disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK &&
> -disk->shared) {
> -if (qemuRemoveSharedDisk(driver->sharedDisks, disk->src) < 0)
> - VIR_WARN("Failed to remove disk '%s' from shared disk table",
> - disk->src);
> +if (ret == 0) {
> +if (qemuRemoveSharedDisk(driver->sharedDisks, disk) < 0)
> +VIR_WARN("Failed to remove disk '%s' from shared disk table",
> + disk->src);
Similar comment here w/r/t NULLSTR and checks in Remove code
> }
>
> return ret;
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 30f923a..bc171a4 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -3453,6 +3453,13 @@ qemuSetUnprivSGIO(virDomainDiskDefPtr disk)
> {
> int val = -1;
>
> +/* "sgio" is only valid for block disk; cdrom
> + * and floopy disk can have empty source.
> + */
> +if (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK ||
> +!disk->src)
> +return 0;
> +
> if (disk->sgio)
> val = (disk->sgio == VIR_DOMAIN_DISK_SGIO_UNFIL
Re: [libvirt] Thoughts on adding a domain type of jvm
On 02/08/2013 12:20 PM, Scott Stark wrote: [any way you can convince your mailer to wrap long lines?] > I'm working with a java vm vendor that is developing a multi-tenancy jvm and > is trying to leverage the libvirt framework for the domain configuration. > Right now the java language binding leverages the libvirt c library for > parsing the domain.xml file. What are the thoughts on adding a 'jvm' domain > type? You mean, adding a a src/jvm/jvm_driver.c backend that supports a jvm:/// URI for connecting to your vms? Seems reasonable; we've added support for other hypervisor technologies in the past. > > I believe there would need to be a jvm specific section in a separate > namespace similar to the qemucmdline section. You may want to look at the patches for adding a URI for parallels:// (commit cafc26ff, and lots of more recent commits with a prefix of 'parallels:') or hyperv:// (commit 4d6e6f4 and more recent commits with a prefix of 'hyperv:') for inspiration on what it would take to contribute your patch. -- 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 14/15] util: maintain caps when running command with uid != 0
On 02/08/2013 02:05 PM, Eric Blake wrote:
> On 02/07/2013 02:37 PM, Laine Stump wrote:
>> virCommand was previously calling virSetUIDGID() to change the uid and
>> gid of the child process, then separately calling
>> virSetCapabilities(). This did not work if the desired uid was != 0,
>> since a setuid to anything other than 0 normally clears all
>> capabilities bits.
>>
>> The solution is to use the new virSetUIDGIDWithCaps(), sending it the
>> uid, gid, and capabilities bits. This will get the new process setup
>> properly.
>>
>> Since the static functions virSetCapabilities() and
>> virClearCapabilities are no longer called, they have been removed.
>>
>> NOTE: When combined with "filecap $path-to-qemu sys_rawio", this patch
>> will make CAP_SYS_RAWIO (which is required for passthrough of generic
>> scsi commands to a guest - see commits e8daeeb, 177db08, 397e6a7, and
>> 74e0349) be retained by qemu when necessary. Apparently that
>> capability has been broken for non-root qemu every since it was
> s/every/ever/
>
>> originally added.
>> ---
>> src/util/vircommand.c | 76
>> ++-
>> 1 file changed, 8 insertions(+), 68 deletions(-)
> ACK.
>
>
>> -# else
>> -static int virClearCapabilities(void)
>> -{
>> -//VIR_WARN("libcap-ng support not compiled in, unable to clear "
>> -// "capabilities");
> Odd that we had commented this out previously.
1) Even more odd is that this function wasn't being called from
anywhere, but still existed *and* didn't cause a compile error. I had
thought that unreferenced static functions always caused a warning
(which we promote to an error).
2) There are several places where we call virCommandClearCapabilities()
without regard to whether or not WITH_CAPNG is actually set. Systems
without WITH_CAPNG would have tons of warnings, which I'm guessing
people don't want. If we want to do a warning or error, maybe we could
do it at configure time (maybe require people to specifically say
"--without-capng" or something).
> Should patch 13/15 log
> any warnings when we are not preserving/clearing capabilities, rather
> than silently ignoring the capability request?
>
>>
>> -if (cmd->uid > 0 || cmd->gid > 0) {
>> -VIR_DEBUG("Setting child uid:gid to %u:%u", cmd->uid, cmd->gid);
>> -if (virSetUIDGID(cmd->uid, cmd->gid) < 0)
>> +/* The steps above may need todo something privileged, so we delay
> As long as you are touching this comment, s/todo/to do/ (but you've
> moved it at least twice in this series, so it depends on how much churn
> you want on when you finally fix it).
Okay. I'll do it here.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 13/15] util: virSetUIDGIDWithCaps - change uid while keeping caps
On 02/08/2013 02:01 PM, Eric Blake wrote:
> On 02/07/2013 02:37 PM, Laine Stump wrote:
>> Normally when a process' uid is changed to non-0, all the capabilities
>> bits are cleared, even those explicitly set with calls to
>> capng_update()/capng_apply() made immediately before setuid. And
>> *after* the process' uid has been changed, it no longer has the
>> necessary privileges to add capabilities back to the process.
>>
>> In order to set a non-0 uid while still maintaining any capabilities
>> bits, it is necessary to either call capng_change_id() (which
>> unfortunately doesn't not currently call initgroups to setup auxiliary
> s/doesn't not/doesn't/
>
>> group membership), or to perform the small amount of calisthenics
>> contained in the new utility function virSetUIDGIDWithCaps().
>>
>> Note that, due to the way that effective capabilities are computed (at
>> time of execve) for a process that has uid != 0, the *file*
>> capabilities of the binary being executed must also have the desired
>> capabilities bit(s) set (see "man 7 capabilities"). This can be done
>> with the "filecap" command
>> (e.g. "filecap /usr/bin/qemu-kvm sys_rawio").
> Sheesh - does that mean that we have to fix up qemu packaging for Fedora
> to include that capability by default?
Well, the kernel guys actually have a patch I'm about to try out that
will hopefully eliminate the need for CAP_COMPROMISE_KERNEL, so we won't
need it for that. And the demand to have generic SCSI command
passthrough is apparently so small that nobody has even tried it (or
possibly they just all gave up and set qemu user to root). But at any
rate, for now I think we can just leave qemu as it is and let people set
the filecap themselves if they need this (unusual) capability; if there
is enough whining, then maybe we can pass it along to qemu.
>
>>
>> +#if WITH_CAPNG
>> +/* Set the real and effective uid and gid to the given values, while
>> + * maintaining the capabilities indicated by bits in @capBits. return
>> + * 0 on success, -1 on failure (the original system error remains in
>> + * errno).
>> + */
>> +int
>> +virSetUIDGIDWithCaps(uid_t uid, gid_t gid, unsigned long long capBits)
>> +{
>> +int ii, capng_ret, ret = -1;
>> +bool need_setgid = false, need_setuid = false;
>> +bool need_prctl = false, need_setpcap = false;
>> +
>> +/* First drop all caps except those in capBits + the extra ones we
>> + * need to change uid/gid and change the capabilities bounding
>> + * set.
>> + */
>> +
>> +capng_clear(CAPNG_SELECT_BOTH);
>> +
>> +for (ii = 0; ii <= CAP_LAST_CAP; ii++) {
>> +if (capBits & (1ULL << ii)) {
>> +capng_update(CAPNG_ADD,
>> + CAPNG_EFFECTIVE|CAPNG_INHERITABLE|
>> + CAPNG_PERMITTED|CAPNG_BOUNDING_SET,
>> + ii);
>> +}
>> +}
>> +
>> +if (gid != -1 && gid > 0 && !capng_have_capability(CAPNG_EFFECTIVE,
>> CAP_SETGID)) {
> Another site that needs casting (gid != (gid_t) -1); and do we really
> want to short-circuit the gid>0 case?
Yeah, I've fixed all of those.
>
>> +need_setgid = true;
>> +capng_update(CAPNG_ADD, CAPNG_EFFECTIVE|CAPNG_PERMITTED,
>> CAP_SETGID);
>> +}
>> +if (uid != -1 && uid > 0 && !capng_have_capability(CAPNG_EFFECTIVE,
>> CAP_SETUID)) {
>> +need_setuid = true;
>> +capng_update(CAPNG_ADD, CAPNG_EFFECTIVE|CAPNG_PERMITTED,
>> CAP_SETUID);
>> +}
>> +# ifdef PR_CAPBSET_DROP
>> +// If newer kernel, we need also need setpcap to change the bounding set
> Prefer /**/ comment style, please.
Oops. That's due to pasting the source of capng_change_id(). I'll fix
all of those.
>
>
>> +}
>> +
>> +#else
>> +/*
>> + * On platforms without libcapng, the capabilities setting is treated
>> + * as a NOP.
>> + */
>> +
>> +int
>> +virSetUIDGIDWithCaps(uid_t uid, gid_t gid, unsigned long long capBits)
> Need ATTRIBUTE_UNUSED on capBits.
Right.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/4] qemu: Merge qemuCheckSharedDisk into qemuAddSharedDisk
On 02/08/2013 08:08 AM, Osier Yang wrote:
> Based on moving various checking into qemuAddSharedDisk, this
> avoids the caller using it in wrong ways.
> ---
> src/qemu/qemu_conf.c| 50
> src/qemu/qemu_driver.c |5
> src/qemu/qemu_process.c | 53
> ---
> src/qemu/qemu_process.h |3 --
> 4 files changed, 50 insertions(+), 61 deletions(-)
>
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index 69ba710..3eeea4b 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -721,6 +721,53 @@ qemuGetSharedDiskKey(const char *disk_path)
> return key;
> }
>
> +/* Check if a shared disk's setting conflicts with the conf
> + * used by other domain(s). Currently only checks the sgio
> + * setting. Note that this should only be called for disk with
> + * block source.
> + *
> + * Returns 0 if no conflicts, otherwise returns -1.
> + */
> +static int
> +qemuCheckSharedDisk(virHashTablePtr sharedDisks,
> +virDomainDiskDefPtr disk)
> +{
> +int val;
> +size_t *ref = NULL;
> +char *key = NULL;
> +int ret = 0;
> +
> +if (!(key = qemuGetSharedDiskKey(disk->src)))
> +return -1;
> +
> +/* It can't be conflict if no other domain is
> + * is sharing it.
> + */
> +if (!(ref = virHashLookup(sharedDisks, key)))
> +goto cleanup;
> +
> +if (virGetDeviceUnprivSGIO(disk->src, NULL, &val) < 0) {
> +ret = -1;
> +goto cleanup;
> +}
> +
> +if ((val == 0 &&
> + (disk->sgio == VIR_DOMAIN_DISK_SGIO_FILTERED ||
> + disk->sgio == VIR_DOMAIN_DISK_SGIO_DEFAULT)) ||
> +(val == 1 &&
> + disk->sgio == VIR_DOMAIN_DISK_SGIO_UNFILTERED))
> +goto cleanup;
> +
> +virReportError(VIR_ERR_OPERATION_INVALID,
> + _("sgio of shared disk '%s' conflicts with other "
> + "active domains"), disk->src);
> +ret = -1;
> +
> +cleanup:
> +VIR_FREE(key);
> +return ret;
> +}
> +
> /* Increase ref count if the entry already exists, otherwise
> * add a new entry.
> */
> @@ -739,6 +786,9 @@ qemuAddSharedDisk(virHashTablePtr sharedDisks,
> !disk->shared || !disk->src)
> return 0;
>
> +if (qemuCheckSharedDisk(sharedDisks, disk) < 0)
> +return -1;
> +
I take the order previously in qemuProcessStart() was wrong then - it
was adding it first, then checking if it was shared. The DiskLive code
checked shared first, then added, which does seem more correct.
However, the change is subtle enough to make me wonder about the reason
for the ordering of calls in the DiskLive code that would check shared
before calls to qemuDomainDetermineDiskChain() and qemuSetupDiskCgroup()
Seems as though now the code in DiskLive may need some extra backout
code if the add now fails because of the check
> if (!(key = qemuGetSharedDiskKey(disk->src)))
> return -1;
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 043efd3..1dc9789 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -5798,11 +5798,6 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
> goto end;
> }
>
> -if (disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK &&
> -disk->shared &&
> -(qemuCheckSharedDisk(driver->sharedDisks, disk) < 0))
> -goto end;
> -
> if (qemuDomainDetermineDiskChain(driver, disk, false) < 0)
> goto end;
>
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index bc171a4..1e0876c 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -3483,56 +3483,6 @@ qemuSetUnprivSGIO(virDomainDiskDefPtr disk)
> return 0;
> }
>
> -/* Check if a shared disk's setting conflicts with the conf
> - * used by other domain(s). Currently only checks the sgio
> - * setting. Note that this should only be called for disk with
> - * block source.
> - *
> - * Returns 0 if no conflicts, otherwise returns -1.
> - */
> -int
> -qemuCheckSharedDisk(virHashTablePtr sharedDisks,
> -virDomainDiskDefPtr disk)
> -{
> -int val;
> -size_t *ref = NULL;
> -char *key = NULL;
> -int ret = 0;
> -
> -if (!(key = qemuGetSharedDiskKey(disk->src)))
> -return -1;
> -
> -/* It can't be conflict if no other domain is
> - * is sharing it.
> - */
> -if (!(ref = virHashLookup(sharedDisks, key)))
> -goto cleanup;
> -
> -if (ref == (void *)0x1)
> -goto cleanup;
> -
> -if (virGetDeviceUnprivSGIO(disk->src, NULL, &val) < 0) {
> -ret = -1;
> -goto cleanup;
> -}
> -
> -if ((val == 0 &&
> - (disk->sgio == VIR_DOMAIN_DISK_SGIO_FILTERED ||
> - disk->sgio == VIR_DOMAIN_DISK_SGIO_DEFAULT)) ||
> -(val == 1 &&
> - disk->sgio == VIR_DOMAIN_DISK_SGIO_UNFILTERED))
> -goto cleanup;
> -
> -virReportError(VIR_ER
Re: [libvirt] [PATCH 3/4] qemu: Don't remove hash entry of other domains
On 02/08/2013 08:08 AM, Osier Yang wrote:
> qemuProcessStart invokes qemuProcessStop when fails, to avoid
> removing hash entry which belongs to other domain(s), this introduces
> a new virBitmapPtr argument for qemuProcessStop. And qemuProcessStart
> sets the bit for the disk only if it's successfully added into the
> hash table. Thus if the argument is provided for qemuProcessStop, it
> can't remove the hash entry belongs to other domain(s).
> ---
> src/qemu/qemu_conf.c |5 -
> src/qemu/qemu_conf.h |3 ++-
> src/qemu/qemu_driver.c| 21 +++--
> src/qemu/qemu_migration.c | 14 +++---
> src/qemu/qemu_process.c | 37 +
> src/qemu/qemu_process.h |3 ++-
> 6 files changed, 55 insertions(+), 28 deletions(-)
>
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index 3eeea4b..a6162b6 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -773,7 +773,8 @@ cleanup:
> */
> int
> qemuAddSharedDisk(virHashTablePtr sharedDisks,
> - virDomainDiskDefPtr disk)
> + virDomainDiskDefPtr disk,
> + int *added)
> {
> size_t *ref = NULL;
> char *key = NULL;
> @@ -804,6 +805,8 @@ qemuAddSharedDisk(virHashTablePtr sharedDisks,
> }
> }
>
> +if (added)
> +*added = 1;
> VIR_FREE(key);
> return 0;
> }
> diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
> index 8e84bcf..b8b5275 100644
> --- a/src/qemu/qemu_conf.h
> +++ b/src/qemu/qemu_conf.h
> @@ -270,7 +270,8 @@ void qemuDriverCloseCallbackRunAll(virQEMUDriverPtr
> driver,
> virConnectPtr conn);
>
> int qemuAddSharedDisk(virHashTablePtr sharedDisks,
> - virDomainDiskDefPtr disk)
> + virDomainDiskDefPtr disk,
> + int *added)
> ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
>
> int qemuRemoveSharedDisk(virHashTablePtr sharedDisks,
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 1dc9789..03fe526 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -2098,7 +2098,7 @@ qemuDomainDestroyFlags(virDomainPtr dom,
> goto endjob;
> }
>
> -qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_DESTROYED, 0);
> +qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_DESTROYED, 0, NULL);
> event = virDomainEventNewFromObj(vm,
> VIR_DOMAIN_EVENT_STOPPED,
> VIR_DOMAIN_EVENT_STOPPED_DESTROYED);
> @@ -2944,7 +2944,7 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver,
> virDomainPtr dom,
> goto endjob;
>
> /* Shut it down */
> -qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_SAVED, 0);
> +qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_SAVED, 0, NULL);
> virDomainAuditStop(vm, "saved");
> event = virDomainEventNewFromObj(vm,
> VIR_DOMAIN_EVENT_STOPPED,
> @@ -3393,7 +3393,7 @@ static int qemuDomainCoreDump(virDomainPtr dom,
>
> endjob:
> if ((ret == 0) && (flags & VIR_DUMP_CRASH)) {
> -qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_CRASHED, 0);
> +qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_CRASHED, 0, NULL);
> virDomainAuditStop(vm, "crashed");
> event = virDomainEventNewFromObj(vm,
> VIR_DOMAIN_EVENT_STOPPED,
> @@ -4902,7 +4902,7 @@ qemuDomainSaveImageStartVM(virConnectPtr conn,
> }
>
> if (virCommandWait(cmd, NULL) < 0) {
> -qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, 0);
> +qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, 0, NULL);
> ret = -1;
> }
> VIR_DEBUG("Decompression binary stderr: %s", NULLSTR(errbuf));
> @@ -5850,7 +5850,7 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
> }
>
> if (ret == 0) {
> -if (qemuAddSharedDisk(driver->sharedDisks, disk) < 0)
> +if (qemuAddSharedDisk(driver->sharedDisks, disk, NULL) < 0)
> VIR_WARN("Failed to add disk '%s' to shared disk table",
> disk->src);
>
> @@ -10677,7 +10677,7 @@ qemuDomainSnapshotCreateActiveInternal(virConnectPtr
> conn,
> if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_HALT) {
> event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED,
>
> VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT);
> -qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, 0);
> +qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, 0,
> NULL);
> virDomainAuditStop(vm, "from-snapshot");
> /* We already filtered the _HALT flag for persistent domains
> * only, so this end job never drops the last reference. */
> @@ -11232,7 +11232,7 @@ qemuDomainSnapshotCreateActiveExternal(virConnect
Re: [libvirt] [PATCH v2 1/4] util: refactor iptables command construction into multiple steps
On 02/08/2013 10:25 AM, Daniel P. Berrange wrote: > On Mon, Feb 04, 2013 at 10:45:23AM +0100, Natanael Copa wrote: >> Instead of creating an iptables command in one shot, do it in steps >> so we can add conditional options like physdev and protocol. >> >> This removes code duplication while keeping existing behaviour. >> >> Signed-off-by: Natanael Copa >> --- >> This patch is unmodified since last time i sent it [1]. >> >> [1] https://www.redhat.com/archives/libvir-list/2012-November/msg00986.html >> >> src/util/viriptables.c | 130 >> ++--- >> 1 file changed, 58 insertions(+), 72 deletions(-) > > ACK, looks fine. Well, almost fine - it had an embedded TAB and failed 'make syntax-check'. But I fixed that with the following squash-in, and pushed: diff --git i/src/util/viriptables.c w/src/util/viriptables.c index b03b0e9..41fe780 100644 --- i/src/util/viriptables.c +++ w/src/util/viriptables.c @@ -1,7 +1,7 @@ /* * viriptables.c: helper APIs for managing iptables * - * Copyright (C) 2007-2012 Red Hat, Inc. + * Copyright (C) 2007-2013 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -397,7 +397,7 @@ iptablesForwardAllowOut(iptablesContext *ctx, action); virCommandAddArgList(cmd, "--source", networkstr, -"--in-interface", iface, NULL); + "--in-interface", iface, NULL); if (physdev && physdev[0]) virCommandAddArgList(cmd, "--out-interface", physdev, NULL); -- 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 v2 0/4] net: support set source address(es) and ports for NAT
On 02/08/2013 10:27 AM, Daniel P. Berrange wrote: > On Mon, Feb 04, 2013 at 10:45:22AM +0100, Natanael Copa wrote: >> This is a rework of the previous sent 'set public ip for nat' patches [1]. >> >> Changes v2: >> - Use separate attributes for addresses and ports as suggested by Laine. >> - support set port range without setting public ip >> > > Broadly looks good to me. If you get rid of patch 2 which is just > a special case of patch 3, then I'd ack 3 + 4. I didn't want to spend time doing the rebase work myself, so I'll wait for a v2 for patches 3 and 4. -- 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 4/4] qemu: Move shared disk entry adding and unpriv_sgio seting
On 02/08/2013 08:08 AM, Osier Yang wrote:
> The disk def could be free'ed by qemuDomainChangeEjectableMedia
> for cdrom or floppy disk. This moves the adding and setting before
> the attaching takes place. And for cdrom floppy disk, if the
> change is ejecting, removing the existed hash entry for it.
> ---
> src/qemu/qemu_driver.c | 23 +--
> src/qemu/qemu_hotplug.c |6 +-
> src/qemu/qemu_hotplug.h |3 ++-
> 3 files changed, 20 insertions(+), 12 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 03fe526..4aad42f 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -5789,6 +5789,7 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
> {
> virDomainDiskDefPtr disk = dev->data.disk;
> virCgroupPtr cgroup = NULL;
> +int eject, added;
Initialize eject and added
> int ret = -1;
>
> if (disk->driverName != NULL && !STREQ(disk->driverName, "qemu")) {
> @@ -5798,6 +5799,12 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
> goto end;
> }
>
> +if (qemuAddSharedDisk(driver->sharedDisks, disk, &added) < 0)
> +goto end;
> +
> +if (qemuSetUnprivSGIO(disk) < 0)
> +goto end;
> +
hrmph. makes my comment in 2/4 less important now. Although it does
make me think that qemuSetUnprivSGIO() could be at the tail end of
qemuAddSharedDisk() since both here and qemuProcessStart() call it.
> if (qemuDomainDetermineDiskChain(driver, disk, false) < 0)
> goto end;
>
> @@ -5814,7 +5821,7 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
> switch (disk->device) {
> case VIR_DOMAIN_DISK_DEVICE_CDROM:
> case VIR_DOMAIN_DISK_DEVICE_FLOPPY:
> -ret = qemuDomainChangeEjectableMedia(driver, vm, disk, false);
> +ret = qemuDomainChangeEjectableMedia(driver, vm, disk, false,
> &eject);
> break;
> case VIR_DOMAIN_DISK_DEVICE_DISK:
> case VIR_DOMAIN_DISK_DEVICE_LUN:
> @@ -5843,22 +5850,18 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
> break;
> }
>
> +if (ret == 0 && eject)
> +ignore_value(qemuRemoveSharedDisk(driver->sharedDisks, disk));
> +
'ret' could be 0 here, but eject undefined... Also we're going to add it
just to remove it?
> if (ret != 0 && cgroup) {
> if (qemuTeardownDiskCgroup(vm, cgroup, disk) < 0)
> VIR_WARN("Failed to teardown cgroup for disk path %s",
> NULLSTR(disk->src));
> }
>
> -if (ret == 0) {
> -if (qemuAddSharedDisk(driver->sharedDisks, disk, NULL) < 0)
> -VIR_WARN("Failed to add disk '%s' to shared disk table",
> - disk->src);
> -
> -if (qemuSetUnprivSGIO(disk) < 0)
> -VIR_WARN("Failed to set unpriv_sgio of disk '%s'", disk->src);
> -}
> -
Rather than move this up to the top, why not swap it with the cgroup
teardown and set ret = qemuAddSharedDisk(); Then the 'eject' code is
perhaps unnecessary. It seems it only exists because you added an
ejectable media to the shared device list and now need to remove it.
> end:
> +if (ret != 0 && added)
You can get here from "if (disk->driverName != NULL &&
!STREQ(disk->driverName, "qemu"))" without added being set and ret == -1
Of course if you take my suggestion to swap cgroup teardown, then this
becomes moot.
> +ignore_value(qemuRemoveSharedDisk(driver->sharedDisks, disk))
> if (cgroup)
> virCgroupFree(&cgroup);
> return ret;
There are two callers to qemuDomainChangeEjectableMedia() in
qemu_driver.c - you only changed one (unless I forgot/missed a prior
patch removing it!).
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 98912bf..18aca47 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -53,7 +53,8 @@
> int qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
> virDomainObjPtr vm,
> virDomainDiskDefPtr disk,
> - bool force)
> + bool force,
> + int *eject)
> {
> virDomainDiskDefPtr origdisk = NULL;
> int i;
> @@ -93,6 +94,9 @@ int qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
> goto cleanup;
> }
>
> +if (eject && origdisk->src && !disk->src)
> +*eject = 1;
> +
> if (virDomainLockDiskAttach(driver->lockManager, cfg->uri,
> vm, disk) < 0)
> goto cleanup;
> diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h
> index 8f01d23..fc0532a 100644
> --- a/src/qemu/qemu_hotplug.h
> +++ b/src/qemu/qemu_hotplug.h
> @@ -31,7 +31,8 @@
> int qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
> virDomainObjPtr vm,
> virDomainDiskDefPtr disk,
> -
Re: [libvirt] [PATCH] fix a bug of sheepdog storage driver
On 02/08/2013 12:26 AM, Osier Yang wrote: > On 2013年02月08日 14:48, harryxi...@gmail.com wrote: Your 'From:' line didn't match your 'Signed-off-by:' line. That makes our generated AUTHORS file look strange. I would suggest doing: git config --global sendemail.from 'Harry Wei ' so that future patches sent through 'git send-email' have the correct envelope settings, and/or edit your gmail settings to associate a name with your address (I'm not sure which of those two steps will matter, or if both are needed). Meanwhile, I touched up the authorship information on this commit. I tweaked the subject line to: sheepdog: skip refresh on creation failure so that it is more informative. Theoretically, ALL patches should either fix a bug or add a feature, so claiming that a patch "fixes a bug" is redundant. Knowing a bit about WHAT was fixed makes the one-line summary much more useful. >> Don't try to refresh Sheepdog volume if creating volume fails. >> >> Signed-off-by: Harry Wei >> >> --- > ACK. Patch now pushed; congratulations on finally getting it right, and hopefully next time isn't quite so difficult. -- 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 1/4] Turn virSecurityManager into a virObjectLockable
On 02/07/2013 10:46 AM, Daniel P. Berrange wrote: > From: "Daniel P. Berrange" > > To enable locking to be introduced to the security manager > objects later, turn virSecurityManager into a virObjectLockable > class > > Signed-off-by: Daniel P. Berrange > --- > src/lxc/lxc_controller.c| 2 +- > src/lxc/lxc_driver.c| 4 ++-- > src/qemu/qemu_conf.h| 2 +- > src/qemu/qemu_driver.c | 6 +++--- > src/security/security_manager.c | 47 > - > src/security/security_manager.h | 2 -- > src/security/security_stack.c | 2 +- > tests/seclabeltest.c| 2 +- > 8 files changed, 46 insertions(+), 21 deletions(-) > ACK -- 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 2/4] Serialize execution of security manager APIs
On 02/07/2013 10:46 AM, Daniel P. Berrange wrote: > From: "Daniel P. Berrange" > > Add locking to virSecurityManagerXXX APIs, so that use of the > security drivers is internally serialized. This avoids the need > to rely on the global driver locks to achieve serialization > > Signed-off-by: Daniel P. Berrange > --- > src/qemu/qemu_conf.h| 2 +- > src/security/security_manager.c | 200 > +++- > 2 files changed, 157 insertions(+), 45 deletions(-) ACK -- 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 1/4] storage: factor out large integer reads
On 02/07/2013 07:53 AM, Daniel P. Berrange wrote:
> On Wed, Feb 06, 2013 at 10:10:17PM -0700, Eric Blake wrote:
>> This makes code easier to read, by avoiding lines longer than
>> 80 columns and removing the repetition from the callers.
>>
>
> How about putting these helpful APIs in some other src/util/ file
> as macros. Either virutil.h, or perhaps virinttypes.h or virendian.h ?
Indeed, that would be nicer, because then it works on both 'char*' and
'unsigned char *' (don't know why virstoragefile uses unsigned, when
most of our other files use plain char).
> Probably with a name like "virReadBufInt{32,64}{BE,LE}"
>
>
> ACK to this patch regardless though.
I'll go ahead and post a v2, with a testsuite addition; it may be a
regression that I'm fixing, but I'd rather get it right than rushed.
--
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] qemu: Fix potential crash when attaching/detaching cdrom or floppy
On 02/07/2013 06:21 AM, Osier Yang wrote: > The crash could happen if the disk source is empty for cdrom or > floppy disk. > --- > src/qemu/qemu_driver.c |7 +-- > src/qemu/qemu_process.c |3 +++ > 2 files changed, 8 insertions(+), 2 deletions(-) I'm not sure if this was subsumed by a later version, but if it still needs a review: ACK. -- 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 3/4] Pass virQEMUDriverPtr into APIs managed shared disk list
On 02/07/2013 10:46 AM, Daniel P. Berrange wrote: > From: "Daniel P. Berrange" > > Currently the APIs for managing the shared disk list take > a virHashTablePtr as the primary argument. This is bad > because it requires the caller to deal with locking of > the QEMU driver. Switch the APIs to take the full > virQEMUDriverPtr instance > > Signed-off-by: Daniel P. Berrange > --- > src/qemu/qemu_conf.c| 51 > +++-- > src/qemu/qemu_conf.h| 4 ++-- > src/qemu/qemu_driver.c | 6 +++--- > src/qemu/qemu_process.c | 10 +- > src/qemu/qemu_process.h | 2 +- > 5 files changed, 35 insertions(+), 38 deletions(-) > ACK. -- 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 00/11] Revisit xen driver Coverity cleanup changes
John Ferlan wrote:
> On 01/30/2013 01:51 PM, John Ferlan wrote:
>
>> src/xen/xen_hypervisor.c | 220 --
>> src/xen/xen_inotify.c| 48
>> src/xen/xend_internal.c | 303
>> ++-
>> src/xen/xm_internal.c| 195 ++
>> src/xen/xs_internal.c| 239 +++--
>> 5 files changed, 325 insertions(+), 680 deletions(-)
>>
>>
>
> Thanks for the reviews Osier and Jim. Rather than respond to each, I'll
> summarize the consistent points. If I should send a v2, let me know.
>
> w/r/t PrivatePtr on one line:
>
> I was trying to go with the 80 column rule. For 99% they could fit on
> one line if I removed one extra space. For a couple, spanning 2 lines
> kept the 80 columns in effect. In looking at other drivers - I don't
> think any of them typecast the *PrivatePtr)->privateData, so I
> could remove that too. I left them in only because they were there.
>
Yeah, good point. And dropping the cast will make all of them fit within
80 columns :).
> w/r/t function headers:
>
> I was trying to be consistent amongst all the files and then more in
> general with other drivers. I kept to the concept that if a header fits
> within 80 columns then don't break up the line. I changed the files
> where a header wouldn't fit into 80 columns to be:
>
> function(type arg,
> type2 arg2,
> type3 arg3,
> type4 arg4)
> {
> }
>
I like the approach and will strive to use that style going forward.
> w/r/t: domain->name:
>
> I wasn't sure with this one, so I had left it in. I removed them.
>
Sounds good.
> There are also checks for "domain->id != -1" - any thoughts on those?
> Those seem to be running or not checks, right? Seems as though the
> virDomainObjIsActive() or hvirCheckFlags() is used elsewhere.
>
Looks like those checks are all done on a virDomainPtr. Probably best to
leave them as is instead of getting a virDomainObjPtr solely for use
with virDomainObjIsActive.
> What about priv->handle in xen_hypervisor.c? Seems as though open will
> set it and everyone else checks it. Given how these drivers call between
> each other, I'm inclined to leave as it, but I'm not against removing
> them either.
>
I tend to agree with leaving as is. In practice, the hypervisor
subdriver is always opened, and if it fails loading the whole xen driver
fails. But in theory opening it could be skipped since it is conditional
on xenHavePrivilege().
Regards,
Jim
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 00/11] Revisit xen driver Coverity cleanup changes
On 02/08/2013 04:15 PM, Jim Fehlig wrote: >> I was trying to go with the 80 column rule. For 99% they could fit on >> one line if I removed one extra space. For a couple, spanning 2 lines >> kept the 80 columns in effect. In looking at other drivers - I don't >> think any of them typecast the *PrivatePtr)->privateData, so I >> could remove that too. I left them in only because they were there. >> > > Yeah, good point. And dropping the cast will make all of them fit within > 80 columns :). The cast is needed in C++; but libvirt is C, and void* automatically casts to whatever type you need without having to be explicit. Go ahead and drop the cast. -- 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] Thoughts on adding a domain type of jvm
- Original Message - From: "Eric Blake" To: "Scott Stark" Cc: libvir-list@redhat.com Sent: Friday, February 8, 2013 12:21:17 PM Subject: Re: [libvirt] Thoughts on adding a domain type of jvm On 02/08/2013 12:20 PM, Scott Stark wrote: > [any way you can convince your mailer to wrap long lines?] Not sure, it is our Red Hat zimbra web client that I'm using. > > I'm working with a java vm vendor that is developing a multi-tenancy jvm > > and is trying to leverage the libvirt framework for the domain > > configuration. Right now the > java language binding leverages the libvirt > > c library for parsing the domain.xml file. What are the thoughts on adding > > a 'jvm' domain type? > > You mean, adding a a src/jvm/jvm_driver.c backend that supports a > jvm:/// URI for connecting to your vms? Seems reasonable; we've added > support for other hypervisor technologies in the past. Yes. > > > > I believe there would need to be a jvm specific section in a separate > > namespace similar to the qemucmdline section. > You may want to look at the patches for adding a URI for parallels:// > (commit cafc26ff, and lots of more recent commits with a prefix of > 'parallels:') or hyperv:// (commit 4d6e6f4 and more recent commits with > a prefix of 'hyperv:') for inspiration on what it would take to > contribute your patch. Ok, will do. Thanks -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/5] Public API to allow defining new domain using OVA file
> NACK, as I said with previous postings, this does not belong in > libvirt APIs, it should be built as a layer above. [AB]: Thanks for reviewing this Daniel. I completly understand the concern of not making it part of libvirt API, but as discussed in other thread (https://www.redhat.com/archives/libvir-list/2012-December/msg00385.html) I was unable to find API supported way to upload OVA disk to the ESX server (which is an integral part of OVA install). >Further this API design fundamentally flawed as it is assuming the hypervisor > driver can access files that are on the libvirt application > machine which is not the case in general. [AB]: If possible please suggest if there can be any other way to overcome this design flaw. Further, there are two unrealted reviews that I posted fixing issues with exisitng ESX driver, it would be useful if someone can review them. 1. https://www.redhat.com/archives/libvir-list/2012-December/msg01377.html 2. https://www.redhat.com/archives/libvir-list/2013-January/msg7.html Thanks!Ata -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] fix a bug of sheepdog storage driver
On Sat, Feb 9, 2013 at 5:40 AM, Eric Blake wrote: > On 02/08/2013 12:26 AM, Osier Yang wrote: >> On 2013年02月08日 14:48, harryxi...@gmail.com wrote: > > Your 'From:' line didn't match your 'Signed-off-by:' line. That makes > our generated AUTHORS file look strange. I would suggest doing: > > git config --global sendemail.from 'Harry Wei ' Thanks, i will execute this command for me. > > so that future patches sent through 'git send-email' have the correct > envelope settings, and/or edit your gmail settings to associate a name > with your address (I'm not sure which of those two steps will matter, or > if both are needed). Meanwhile, I touched up the authorship information > on this commit. > > I tweaked the subject line to: > > sheepdog: skip refresh on creation failure > It looks better, thanks. > so that it is more informative. Theoretically, ALL patches should > either fix a bug or add a feature, so claiming that a patch "fixes a > bug" is redundant. Knowing a bit about WHAT was fixed makes the > one-line summary much more useful. Thanks, i will give more informative one-line summary for later patches. > >>> Don't try to refresh Sheepdog volume if creating volume fails. >>> >>> Signed-off-by: Harry Wei >>> >>> --- > >> ACK. > > Patch now pushed; congratulations on finally getting it right, and > hopefully next time isn't quite so difficult. > Thanks for your jobs ;-) -- Thanks Harry Wei -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
