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

Re: [libvirt] [PATCH 5/7] Add a port allocator class



On Tue, Jan 15, 2013 at 05:30:23PM -0700, Eric Blake wrote:
> On 01/11/2013 05:13 AM, Daniel P. Berrange wrote:
> > +
> > +    unsigned int start;
> > +    unsigned int end;
> > +};
> 
> > +
> > +virPortAllocatorPtr virPortAllocatorNew(unsigned short start,
> > +                                        unsigned short end)
> > +
> > +{
> 
> Spurious blank line.  Any reason you allocate with short, but store the
> values in int internally?  (unsigned short in the struct should be fine)

Yes, using a short in the struct would be better & was my
intention.

> > +    virPortAllocatorPtr pa;
> > +
> > +    if (start >= end) {
> > +        virReportInvalidArg(start, "start port %d must be less than end port %d",
> > +                            start, end);
> > +        return NULL;
> > +    }
> 
> Since this error gave a message,
> 
> > +
> > +    if (virPortAllocatorInitialize() < 0)
> > +        return NULL;
> > +
> > +    if (!(pa = virObjectLockableNew(virPortAllocatorClass)))
> > +        return NULL;
> 
> does this error need to call virReportOOMError()?

This function raises an error already.

> 
> > +
> > +    pa->start = start;
> > +    pa->end = end;
> > +
> > +    if (!(pa->bitmap = virBitmapNew(end-start))) {
> > +        virObjectUnref(pa);
> > +        return NULL;
> 
> Same question here.  Callers can't tell if a NULL return means OOM or
> usage error.

I thought this did too, but it appears to leave it upto the
caller, so I'll add it here.


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|


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