On Wed, Mar 18, 2009 at 11:00:13PM +0100, "Abel M?guez Rodr?guez" wrote:
> > On Wed, Mar 18, 2009 at 01:52:56PM +0100, "Abel Míguez
> > Rodríguez" wrote:
> > Can you repost that from a normail mail client and without HTML
> > formatting ... it's simply unreadable for me,
> >
>
> Of course, I repost the e-mail using plain text, but (right now) I have to use a web client.
> virCapsPtr oneCapsInit(void)
> {
> struct utsname utsname;
> virCapsPtr caps;
> virCapsGuestPtr guest;
>
> uname(&utsname);
>
> if ((caps = virCapabilitiesNew(utsname.machine,0,0)) == NULL)
> {
> goto no_memory;
> }
>
> virCapabilitiesSetMacPrefix(caps,(unsigned char[]){ 0x52, 0x54, 0x00 });
>
> if ((guest = virCapabilitiesAddGuest(caps,
> "hvm",
> utsname.machine,
> sizeof(int) == 4 ? 32 : 64,
> NULL,
> NULL,
> 0,
> NULL)) == NULL)
> {
> goto no_memory;
> }
What architectures does OpenNebular support ? I'm guessing i686 and x86_64
at least ? Since x86_64 supports running i686 guests, you may well want
to register a second guest here for 'i686' arch, if you have a x86_64 host.
>
> if (virCapabilitiesAddGuestDomain(guest,
> "hvm",
> NULL,
> NULL,
> 0,
> NULL) == NULL)
The guest domain is refering to the type of virtualization used, eg 'xen', 'qemu',
'kvm', etc. So 'hvm' is wrong in this context. Since OpenNebular will push out the
vm to any host, with varying hypervisors, its probably best to just invent a generic
domain called "one" here.
> int oneSubmitVM(virConnectPtr conn ATTRIBUTE_UNUSED,
> one_driver_t* driver,
> virDomainObjPtr vm)
> {
> FILE* fd=NULL;
> char path[256];
> int oneid;
>
> snprintf(path,256,"/tmp/one-%d",driver->nextid);
Creating files in /tmp with predictable names is a real security flaw.
Is there no way to create the VM without having to use a temporary
file on disk ? eg, just do everything in memory ?
If not, then at least use the mkstemp() function for creating the
file more safely.
> int xmlOneTemplate(virDomainDefPtr def, FILE* fd)
> {
> char* buf;
> int i;
>
> fprintf(fd,"#OpenNebula Template automatically generated by libvirt\n");
>
> fprintf(fd,"NAME = %s\n",def->name);
> fprintf(fd,"CPU = %ld\n",(def->vcpus));
> fprintf(fd,"MEMORY = %ld\n",(def->maxmem)/1024);
>
> /*Optional Booting OpenNebula Information:*/
>
> if( def->os.kernel )
> {
> fprintf(fd,"OS=[ kernel = \"%s\"",def->os.kernel);
>
> if(def->os.initrd)
> fprintf(fd,",\n initrd = \"%s\"",def->os.initrd);
> if(def->os.cmdline)
> fprintf(fd,",\n kernel_cmd = \"%s\"",def->os.cmdline);
> if(def->os.root)
> fprintf(fd,",\n root = \"%s\"",def->os.root);
>
> fprintf(fd," ]\n");
> }
>
> /* set Disks & NICS */
>
> for(i=0 ; i<def->ndisks ; i++)
> {
> if ( !def->disks[i] || !def->disks[i]->src || !def->disks[i]->src )
> {
> continue;
> }
I think the last check in this if, should be against '->dst' rather than
duplicating ->src again ?
Also, I imagine you may also want to do something with device type, eg
disk, vs cdrom, vs floppy.
> fprintf(fd, "DISK=[ source = \"%s\",\n"
> " target = \"%s\",\n"
> " readonly =",
> def->disks[i]->src,
> def->disks[i]->dst);
>
> if(def->disks[i]->readonly)
> fprintf(fd,"\"yes\"]\n");
> else
> fprintf(fd,"\"no\"]\n");
> }
>
> for(i=0 ; i< def->nnets ; i++)
> {
> if ( !def->nets[i] )
> {
> continue;
> }
>
> switch(def->nets[i]->type)
> {
> case VIR_DOMAIN_NET_TYPE_BRIDGE:
> fprintf(fd,"NIC=[ bridge =\"%s\",\n",def->nets[i]->data.bridge.brname);
> if(def->nets[i]->ifname)
> {
> fprintf(fd," target =\"%s\",\n",def->nets[i]->ifname);
> }
>
> fprintf(fd," mac =\"%02x:%02x:%02x:%02x:%02x:%02x\" ]\n",
> def->nets[i]->mac[0],def->nets[i]->mac[1],
> def->nets[i]->mac[2],def->nets[i]->mac[3],
> def->nets[i]->mac[4],def->nets[i]->mac[5]);
> break;
>
> case VIR_DOMAIN_NET_TYPE_NETWORK:
> fprintf(fd,"NIC=[ network=\"%s\" ]\n",def->nets[i]->data.network.name);
> break;
> }
You have a 'default:' block here that raises an error if any other type of
network is requested, so the user sees they did something unsupported, rather
than having their network silently ignored.
> #include "virterror_internal.h"
> #include "logging.h"
> #include "datatypes.h"
> #include "one_conf.h"
> #include "one_driver.h"
> #include "memory.h"
> #include "util.h"
> #include "bridge.h"
> #include "veth.h"
> #include "event.h"
> #include "cgroup.h"
I don't think need the 'event.h' or 'cgroup.h' includes here, since
you're not calling any of their functions.
>
> static virDrvOpenStatus oneOpen(virConnectPtr conn,
> virConnectAuthPtr auth ATTRIBUTE_UNUSED,
> int flags ATTRIBUTE_UNUSED)
> {
> /* Verify uri was specified */
> if (conn->uri == NULL) {
> conn->uri = xmlParseURI("one:///");
> if (!conn->uri) {
> oneError(conn, NULL, VIR_ERR_NO_MEMORY, NULL);
We no longer use VIR_ERR_NO_MEMORY directly - all those places should
use 'virReportOOMError(conn)'. If you run 'make syntax-check' it will
tell you all the places using VIR_ERR_NO_MEMORY that need to be changed
> static int oneListDomains(virConnectPtr conn, int *ids, int nids)
> {
> one_driver_t *driver = (one_driver_t *)conn->privateData;
> int got = 0, i;
>
> oneDriverLock(driver);
> for (i = 0 ; i < driver->domains.count && got < nids ; i++){
> if (virDomainIsActive(driver->domains.objs[i]))
> ids[got++] = driver->domains.objs[i]->def->id;
> }
> oneDriverUnlock(driver);
>
> return got;
> }
>
> static int oneNumDomains(virConnectPtr conn)
> {
> one_driver_t *driver = (one_driver_t *)conn->privateData;
> int n = 0, i;
>
> oneDriverLock(driver);
> for (i = 0 ; i < driver->domains.count ; i++)
> if (virDomainIsActive(driver->domains.objs[i]))
> n++;
> oneDriverUnlock(driver);
>
> return n;
> }
>
> static int oneListDefinedDomains(virConnectPtr conn,
> char **const names, int nnames) {
> one_driver_t *driver = (one_driver_t *)conn->privateData;
> int got = 0, i;
>
> oneDriverLock(driver);
> for (i = 0 ; i < driver->domains.count && got < nnames ; i++) {
> if (!virDomainIsActive(driver->domains.objs[i])) {
> if (!(names[got++] = strdup(driver->domains.objs[i]->def->name))) {
> oneError(conn, NULL, VIR_ERR_NO_MEMORY,
> "%s", _("failed to allocate space for VM name string"));
> goto cleanup;
> }
> }
> }
> oneDriverUnlock(driver);
>
> return got;
>
> cleanup:
> for (i = 0 ; i < got ; i++)
> VIR_FREE(names[i]);
> oneDriverUnlock(driver);
>
> return -1;
> }
>
> static int oneNumDefinedDomains(virConnectPtr conn)
> {
> one_driver_t *driver = (one_driver_t *)conn->privateData;
> int n = 0, i;
>
> oneDriverLock(driver);
> for (i = 0 ; i < driver->domains.count ; i++)
> if (!virDomainIsActive(driver->domains.objs[i]))
> n++;
> oneDriverUnlock(driver);
>
> return n;
> }
In all these 4 functions, to be thread-safe you need to
call virDomainObjLock(driver->domains.objs[i]) before
accessing the individual domain objects, and unlock it
afterwards. Take a look at the qemu_driver.c file's equivalent
methods for examples.
Regards,
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list