[libvirt] Re: OpenVZ : The restriction of domain name should be addressed
Hi Daniel, I didn't realize that I even did not follow the manner of XML. I have worked with this problem and got a small patch to handle "ID" in OpenVZ functionality. I found it is working well with the XML script included "ID" in domain tag. I am concerned that I had to edit the common file domain_conf.c. I still believe I should keep it away from this problem for compatibility with the others. How do you think can I avoid this? diff --git a/src/domain_conf.c b/src/domain_conf.c index 5ae0775..f74961f 100644 --- a/src/domain_conf.c +++ b/src/domain_conf.c @@ -2492,7 +2492,7 @@ static virDomainDefPtr virDomainDefParseXML (virConnectPtr conn, return NULL; } - if (!(flags & VIR_DOMAIN_XML_INACTIVE)) + // if (!(flags & VIR_DOMAIN_XML_INACTIVE)) if((virXPathLong(conn, "string(./@id)", ctxt, &id)) < 0) id = -1; def->id = (int)id; diff --git a/src/openvz_conf.c b/src/openvz_conf.c index be94b9e..d23f173 100644 --- a/src/openvz_conf.c +++ b/src/openvz_conf.c @@ -425,17 +425,18 @@ int openvzLoadDomains(struct openvz_driver *driver) { char uuidstr[VIR_UUID_STRING_BUFLEN]; virDomainObjPtr dom = NULL; char temp[50]; + char name[64]; if (openvzAssignUUIDs() < 0) return -1; - if ((fp = popen(VZLIST " -a -ovpsid,status -H 2>/dev/null", "r")) == NULL) { + if ((fp = popen(VZLIST " -a -ovpsid,name,status -H 2>/dev/null", "r")) == NULL) { openvzError(NULL, VIR_ERR_INTERNAL_ERROR, "%s", _("popen failed")); return -1; } while(!feof(fp)) { - if (fscanf(fp, "%d %s¥n", &veid, status) != 2) { + if (fscanf(fp, "%d %s¥n", &veid, name, status) != 3) { if (feof(fp)) break; @@ -465,7 +466,7 @@ int openvzLoadDomains(struct openvz_driver *driver) { dom->pid = veid; dom->def->id = dom->state == VIR_DOMAIN_SHUTOFF ? -1 : veid; - if (virAsprintf(&dom->def->name, "%i", veid) < 0) + if (virAsprintf(&dom->def->name, "%s", name) < 0) goto no_memory; openvzGetVPSUUID(veid, uuidstr, sizeof(uuidstr)); diff --git a/src/openvz_driver.c b/src/openvz_driver.c index a8c24ba..c0c1e0f 100644 --- a/src/openvz_driver.c +++ b/src/openvz_driver.c @@ -101,6 +101,7 @@ static int openvzDomainDefineCmd(virConnectPtr conn, virDomainDefPtr vmdef) { int narg; + char str_id[10]; for (narg = 0; narg < maxarg; narg++) args[narg] = NULL; @@ -130,6 +131,11 @@ static int openvzDomainDefineCmd(virConnectPtr conn, ADD_ARG_LIT(VZCTL); ADD_ARG_LIT("--quiet"); ADD_ARG_LIT("create"); + + sprintf( str_id, "%d", vmdef->id ); + ADD_ARG_LIT(str_id); + + ADD_ARG_LIT("--name"); ADD_ARG_LIT(vmdef->name); if (vmdef->nfss == 1 && @@ -1229,7 +1235,11 @@ static int openvzListDefinedDomains (virConnectPtr conn, char vpsname[32]; char buf[32]; char *endptr; - const char *cmd[] = {VZLIST, "-ovpsid", "-H", "-S", NULL}; + const char *cmd[] = {VZLIST, "-oname", "-H", "-S", NULL}; + int cnt = 0; + char name_buf[32]; + + /* the -S options lists only stopped domains */ ret = virExec(conn, cmd, NULL, NULL, @@ -1241,14 +1251,14 @@ static int openvzListDefinedDomains (virConnectPtr conn, } while(got < nnames){ - ret = openvz_readline(outfd, buf, 32); + ret = openvz_readline(outfd, buf, 64); if(!ret) break; - if (virStrToLong_i(buf, &endptr, 10, &veid) < 0) { - openvzError(conn, VIR_ERR_INTERNAL_ERROR, - _("Could not parse VPS ID %s"), buf); - continue; + cnt = 0; + while( buf[cnt] != ' ' ){ + name_buf[cnt] = buf[cnt]; + cnt++; } - snprintf(vpsname, sizeof(vpsname), "%d", veid); + snprintf(vpsname, sizeof(vpsname), "%d", name_buf); if (!(names[got] = strdup(vpsname))) goto no_memory; got ++; -- 1.5.2.2 ----- Yuji Nishida nishidy@nict.go.jp
On Tue, Sep 15, 2009 at 03:40:09PM +0900, Yuji NISHIDA wrote:
Hi Daniel,
I didn't realize that I even did not follow the manner of XML. I have worked with this problem and got a small patch to handle "ID" in OpenVZ functionality. I found it is working well with the XML script included "ID" in domain tag.
I am concerned that I had to edit the common file domain_conf.c. I still believe I should keep it away from this problem for compatibility with the others. How do you think can I avoid this?
The 'id' value is not intended to be settable by the end user, which is why the domain_conf.c parser does not parse it by default. So for your usage in OpenVZ, you'll need to auto-assign an 'id' value when creating a new guest. eg, get a list of existing OpenVZ guest 'id' values and then pick the first one which is not used.
diff --git a/src/openvz_driver.c b/src/openvz_driver.c index a8c24ba..c0c1e0f 100644 --- a/src/openvz_driver.c +++ b/src/openvz_driver.c @@ -130,6 +131,11 @@ static int openvzDomainDefineCmd(virConnectPtr conn, ADD_ARG_LIT(VZCTL); ADD_ARG_LIT("--quiet"); ADD_ARG_LIT("create"); + + sprintf( str_id, "%d", vmdef->id ); + ADD_ARG_LIT(str_id); + + ADD_ARG_LIT("--name"); ADD_ARG_LIT(vmdef->name);
This is where you need to pull in an auto-assigned ID value instead of using vmdef->id. 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 :|
Hi Daniel Fixed patch according to your comments in openvzDomainDefineCmd func. --- a/src/openvz_driver.c +++ b/src/openvz_driver.c @@ -101,6 +101,9 @@ static int openvzDomainDefineCmd(virConnectPtr conn, virDomainDefPtr vmdef) { int narg; + int veid; + int max_veid; + FILE *fp; for (narg = 0; narg < maxarg; narg++) args[narg] = NULL; @@ -130,6 +133,38 @@ static int openvzDomainDefineCmd(virConnectPtr conn, ADD_ARG_LIT(VZCTL); ADD_ARG_LIT("--quiet"); ADD_ARG_LIT("create"); + + if ((fp = popen(VZLIST " -a -ovpsid -H 2>/dev/null", "r")) == NULL) { + openvzError(NULL, VIR_ERR_INTERNAL_ERROR, "%s", _("popen failed")); + return -1; + } + max_veid = 0; + while(!feof(fp)) { + if (fscanf(fp, "%d\n", &veid ) != 1) { + if (feof(fp)) + break; + + openvzError(NULL, VIR_ERR_INTERNAL_ERROR, + "%s", _("Failed to parse vzlist output")); + goto cleanup; + } + if(veid > max_veid){ + max_veid = veid; + } + } + fclose(fp); + + if(max_veid == 0){ + max_veid = 100; + }else{ + max_veid++; + } + + char str_id[10]; + sprintf( str_id, "%d", max_veid++ ); + ADD_ARG_LIT(str_id); + + ADD_ARG_LIT("--name"); ADD_ARG_LIT(vmdef->name); if (vmdef->nfss == 1 && @@ -151,6 +186,11 @@ static int openvzDomainDefineCmd(virConnectPtr conn, openvzError(conn, VIR_ERR_INTERNAL_ERROR, _("Could not put argument to %s"), VZCTL); return -1; + + cleanup: + fclose(fp); + return -1; + #undef ADD_ARG #undef ADD_ARG_LIT } ----- Yuji Nishida nishidy@nict.go.jp On 2009/09/15, at 18:59, Daniel P. Berrange wrote:
On Tue, Sep 15, 2009 at 03:40:09PM +0900, Yuji NISHIDA wrote:
Hi Daniel,
I didn't realize that I even did not follow the manner of XML. I have worked with this problem and got a small patch to handle "ID" in OpenVZ functionality. I found it is working well with the XML script included "ID" in domain tag.
I am concerned that I had to edit the common file domain_conf.c. I still believe I should keep it away from this problem for compatibility with the others. How do you think can I avoid this?
The 'id' value is not intended to be settable by the end user, which is why the domain_conf.c parser does not parse it by default.
So for your usage in OpenVZ, you'll need to auto-assign an 'id' value when creating a new guest. eg, get a list of existing OpenVZ guest 'id' values and then pick the first one which is not used.
diff --git a/src/openvz_driver.c b/src/openvz_driver.c index a8c24ba..c0c1e0f 100644 --- a/src/openvz_driver.c +++ b/src/openvz_driver.c @@ -130,6 +131,11 @@ static int openvzDomainDefineCmd(virConnectPtr conn, ADD_ARG_LIT(VZCTL); ADD_ARG_LIT("--quiet"); ADD_ARG_LIT("create"); + + sprintf( str_id, "%d", vmdef->id ); + ADD_ARG_LIT(str_id); + + ADD_ARG_LIT("--name"); ADD_ARG_LIT(vmdef->name);
This is where you need to pull in an auto-assigned ID value instead of using vmdef->id.
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 :|
Yuji NISHIDA wrote:
Hi Daniel
Fixed patch according to your comments in openvzDomainDefineCmd func.
--- a/src/openvz_driver.c +++ b/src/openvz_driver.c @@ -101,6 +101,9 @@ static int openvzDomainDefineCmd(virConnectPtr conn, virDomainDefPtr vmdef) { int narg; + int veid; + int max_veid; + FILE *fp;
for (narg = 0; narg < maxarg; narg++) args[narg] = NULL; @@ -130,6 +133,38 @@ static int openvzDomainDefineCmd(virConnectPtr conn, ADD_ARG_LIT(VZCTL); ADD_ARG_LIT("--quiet"); ADD_ARG_LIT("create"); + + if ((fp = popen(VZLIST " -a -ovpsid -H 2>/dev/null", "r")) == NULL) { + openvzError(NULL, VIR_ERR_INTERNAL_ERROR, "%s", _("popen failed")); + return -1; + } + max_veid = 0; + while(!feof(fp)) { + if (fscanf(fp, "%d\n", &veid ) != 1) { + if (feof(fp)) + break; + + openvzError(NULL, VIR_ERR_INTERNAL_ERROR, + "%s", _("Failed to parse vzlist output")); + goto cleanup; + } + if(veid > max_veid){ + max_veid = veid; + } + } + fclose(fp); + + if(max_veid == 0){ + max_veid = 100; + }else{ + max_veid++; + }
You might want to add a comment saying that vpsid's below 100 are reserved for OpenVZ internal use; otherwise, it looks like an odd place to begin numbering.
+ + char str_id[10]; + sprintf( str_id, "%d", max_veid++ );
You'll want to use snprintf here, like: snprintf(str_id, sizeof(str_id), "%d", max_veid++); (bear with me on this part, since I don't know much about OpenVZ). Besides that, though, I'm not sure you necessarily want to do it like this, since you aren't really tracking the ID's properly. The problem I see is that if you do it like this, start the container, and then do "virsh dumpxml <openvz>", you won't see the ID in the output XML, like you do for the other drivers. Is that intentional? If not, I think you'll want to store the id in the virDomainDef->id member so that the information will be properly printed to the user. -- Chris Lalancette
On Wed, Sep 23, 2009 at 10:22:51AM +0200, Chris Lalancette wrote:
Yuji NISHIDA wrote:
Hi Daniel
Fixed patch according to your comments in openvzDomainDefineCmd func.
--- a/src/openvz_driver.c +++ b/src/openvz_driver.c @@ -101,6 +101,9 @@ static int openvzDomainDefineCmd(virConnectPtr conn, virDomainDefPtr vmdef) { int narg; + int veid; + int max_veid; + FILE *fp;
for (narg = 0; narg < maxarg; narg++) args[narg] = NULL; @@ -130,6 +133,38 @@ static int openvzDomainDefineCmd(virConnectPtr conn, ADD_ARG_LIT(VZCTL); ADD_ARG_LIT("--quiet"); ADD_ARG_LIT("create"); + + if ((fp = popen(VZLIST " -a -ovpsid -H 2>/dev/null", "r")) == NULL) { + openvzError(NULL, VIR_ERR_INTERNAL_ERROR, "%s", _("popen failed")); + return -1; + } + max_veid = 0; + while(!feof(fp)) { + if (fscanf(fp, "%d\n", &veid ) != 1) { + if (feof(fp)) + break; + + openvzError(NULL, VIR_ERR_INTERNAL_ERROR, + "%s", _("Failed to parse vzlist output")); + goto cleanup; + } + if(veid > max_veid){ + max_veid = veid; + } + } + fclose(fp); + + if(max_veid == 0){ + max_veid = 100; + }else{ + max_veid++; + }
You might want to add a comment saying that vpsid's below 100 are reserved for OpenVZ internal use; otherwise, it looks like an odd place to begin numbering.
good point.
+ + char str_id[10]; + sprintf( str_id, "%d", max_veid++ );
You'll want to use snprintf here, like:
snprintf(str_id, sizeof(str_id), "%d", max_veid++);
(bear with me on this part, since I don't know much about OpenVZ).
Besides that, though, I'm not sure you necessarily want to do it like this, since you aren't really tracking the ID's properly. The problem I see is that if you do it like this, start the container, and then do "virsh dumpxml <openvz>", you won't see the ID in the output XML, like you do for the other drivers. Is that intentional? If not, I think you'll want to store the id in the virDomainDef->id member so that the information will be properly printed to the user.
I actually applied that patch on monday (after a couple of cleanups) and apparently my reply mail is part of the set that got lost :-( Author: Yuji NISHIDA <nishidy@nict.go.jp> 2009-09-22 12:19:09 Committer: Daniel Veillard <veillard@redhat.com> 2009-09-22 12:19:09 0c85095e46f3aba09ac401f559b76df0b0bea998 the snprintf wasn't looking critical because I don't think a %d can generate more than 9 characters, but you're right in the absolute :-) Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
Thanks, Chris and Daniel I corrected the code that I posted here according to your comments. Chris, I now need to handle openvz containers by character(name) not integer(id) at all. diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index 54bcaa9..3b8505d 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -156,12 +156,13 @@ openvzDomainDefineCmd(virConnectPtr conn, fclose(fp); if (max_veid == 0) { - max_veid = 100; + /* OpenVZ reserves the IDs ranging from 0 to 100 */ + max_veid = 101; } else { max_veid++; } - sprintf(str_id, "%d", max_veid++); + snprintf(str_id, sizeof(str_id), "%d", max_veid); ADD_ARG_LIT(str_id); ADD_ARG_LIT("--name"); -- 1.5.2.2 ----- Yuji Nishida nishidy@nict.go.jp On 2009/09/23, at 23:45, Daniel Veillard wrote:
On Wed, Sep 23, 2009 at 10:22:51AM +0200, Chris Lalancette wrote:
Yuji NISHIDA wrote:
Hi Daniel
Fixed patch according to your comments in openvzDomainDefineCmd func.
--- a/src/openvz_driver.c +++ b/src/openvz_driver.c @@ -101,6 +101,9 @@ static int openvzDomainDefineCmd(virConnectPtr conn, virDomainDefPtr vmdef) { int narg; + int veid; + int max_veid; + FILE *fp;
for (narg = 0; narg < maxarg; narg++) args[narg] = NULL; @@ -130,6 +133,38 @@ static int openvzDomainDefineCmd(virConnectPtr conn, ADD_ARG_LIT(VZCTL); ADD_ARG_LIT("--quiet"); ADD_ARG_LIT("create"); + + if ((fp = popen(VZLIST " -a -ovpsid -H 2>/dev/null", "r")) == NULL) { + openvzError(NULL, VIR_ERR_INTERNAL_ERROR, "%s", _("popen failed")); + return -1; + } + max_veid = 0; + while(!feof(fp)) { + if (fscanf(fp, "%d\n", &veid ) != 1) { + if (feof(fp)) + break; + + openvzError(NULL, VIR_ERR_INTERNAL_ERROR, + "%s", _("Failed to parse vzlist output")); + goto cleanup; + } + if(veid > max_veid){ + max_veid = veid; + } + } + fclose(fp); + + if(max_veid == 0){ + max_veid = 100; + }else{ + max_veid++; + }
You might want to add a comment saying that vpsid's below 100 are reserved for OpenVZ internal use; otherwise, it looks like an odd place to begin numbering.
good point.
+ + char str_id[10]; + sprintf( str_id, "%d", max_veid++ );
You'll want to use snprintf here, like:
snprintf(str_id, sizeof(str_id), "%d", max_veid++);
(bear with me on this part, since I don't know much about OpenVZ).
Besides that, though, I'm not sure you necessarily want to do it like this, since you aren't really tracking the ID's properly. The problem I see is that if you do it like this, start the container, and then do "virsh dumpxml <openvz>", you won't see the ID in the output XML, like you do for the other drivers. Is that intentional? If not, I think you'll want to store the id in the virDomainDef->id member so that the information will be properly printed to the user.
I actually applied that patch on monday (after a couple of cleanups) and apparently my reply mail is part of the set that got lost :-( Author: Yuji NISHIDA <nishidy@nict.go.jp> 2009-09-22 12:19:09 Committer: Daniel Veillard <veillard@redhat.com> 2009-09-22 12:19:09 0c85095e46f3aba09ac401f559b76df0b0bea998
the snprintf wasn't looking critical because I don't think a %d can generate more than 9 characters, but you're right in the absolute :-)
Daniel
-- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
Yuji NISHIDA wrote:
Thanks, Chris and Daniel
I corrected the code that I posted here according to your comments. Chris, I now need to handle openvz containers by character(name) not integer(id) at all.
Right, the patch looks good to correct the two smaller issues that I pointed out, but you still have the problem of not tracking the ID's properly. I guess that will have to be done separately. -- Chris Lalancette
participants (4)
-
Chris Lalancette -
Daniel P. Berrange -
Daniel Veillard -
Yuji NISHIDA