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

[Daniel Veillard]

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