Re: [libvirt] Re: OpenVZ : The restriction of domain name should be addressed

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(a)nict.go.jp&gt; 2009-09-22 12:19:09 Committer: Daniel Veillard <veillard(a)redhat.com&gt; 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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/