Re: PATCH 1/2 QEMU driver - internal driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Feb 13, 2007 at 04:06:17PM -0500, Daniel Veillard wrote:
> On Tue, Feb 13, 2007 at 07:03:42PM +0000, Daniel P. Berrange wrote:
> 
> > +    /* Block sending entire outgoing packet */
> > +    while (outLeft) {
> > +        int got = write(conn->handle, out+outDone, outLeft);
> 
>   stylistic, I prefer to have the variables defined at the function level,
> but I could understand why one would argue otherwise :-)
> Appears in many other places, definitely not a blocker though !

Personally I'd define them at point of first use, but we're not using
C99 in libvirt, so I'll settle for start of nearest nested code block

> > +    nDomains = reply.data.listDomainsReply.numDomains;
> > +    if (nDomains > maxids)
> > +        return -1;
> 
>   Is the semantic really to error instead of truncating in that case ?
> it seems to me the code in other drivers just pass the first maxids ones.

Yeah, I'll just clamp it to maxids.

> > +virDomainPtr
> > +qemuDomainCreateLinux(virConnectPtr conn, const char *xmlDesc,
> > +                       unsigned int flags ATTRIBUTE_UNUSED){
> > +    struct qemud_packet req, reply;
> > +    virDomainPtr dom;
> > +    int len = strlen(xmlDesc);
> > +
> > +    if (len > (QEMUD_MAX_XML_LEN-1)) {
> 
>   maybe we need to provide a clear error there

Definitely. There's a bunch of other places which need more error reporting
in both driver & daemon. I've fixed up all the critical path ones which you
will encounter commonly, but still more low priority ones to deal with.

> > +int qemuListDefinedDomains(virConnectPtr conn,
> > +int qemuDomainCreate(virDomainPtr dom) {
> > +virDomainPtr qemuDomainDefineXML(virConnectPtr conn, const char *xml) {
> > +int qemuUndefine(virDomainPtr dom) {
> 
> Seems to me all of these drivers entry point should be made static since
> they are not exported from the .h, right ? Only the registration routine
> ought to be exported (very clean :-).

Sounds reasonable.

Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 


[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]