[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 01/11/2013 05:13 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange redhat com>
> 
> Introduce a virPortAllocator for managing TCP port allocations.
> ---
>  .gitignore                   |   1 +
>  src/Makefile.am              |   1 +
>  src/libvirt_private.syms     |   6 ++
>  src/util/virportallocator.c  | 188 +++++++++++++++++++++++++++++++++++++++++
>  src/util/virportallocator.h  |  40 +++++++++
>  tests/Makefile.am            |  17 +++-
>  tests/virportallocatortest.c | 195 +++++++++++++++++++++++++++++++++++++++++++
>  7 files changed, 447 insertions(+), 1 deletion(-)
>  create mode 100644 src/util/virportallocator.c
>  create mode 100644 src/util/virportallocator.h
>  create mode 100644 tests/virportallocatortest.c
> 

> +
> +    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)

> +    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()?

> +
> +    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.


> +++ b/tests/Makefile.am
> @@ -97,6 +97,7 @@ test_programs = virshtest sockettest \
>  	virbitmaptest \
>  	virlockspacetest \
>  	virstringtest \
> +        virportallocatortest \

Space vs. tab issue (one of the few files where we prefer tab).

> +
> +VIRT_TEST_MAIN_PRELOAD(mymain, abs_builddir "/.libs/libvirportallocatormock.so")

Nice way to fake out bind() - this PRELOAD test idiom is turning out to
be useful.

ACK if you address the points above.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


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