1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH 0/6] port allocator: make used port bitmap global etc
  4. View patch

[libvirt] [PATCH 2/6] port allocator: remove range on manual port reserving

Nikolay Shirokovskiy posted 6 patches 7 years, 4 months ago
There is a newer version of this series
[libvirt] [PATCH 2/6] port allocator: remove range on manual port reserving
Posted by Nikolay Shirokovskiy 7 years, 4 months ago 
Range check in virPortAllocatorSetUsed is not useful anymore
when we manage ports for entire unsigned short range values.
---
 src/bhyve/bhyve_command.c   |  4 +---
 src/bhyve/bhyve_process.c   |  4 +---
 src/qemu/qemu_process.c     | 41 +++++++++++------------------------------
 src/util/virportallocator.c |  9 +--------
 src/util/virportallocator.h |  4 +---
 5 files changed, 15 insertions(+), 47 deletions(-)

diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c
index 55032ae..9d21788 100644
--- a/src/bhyve/bhyve_command.c
+++ b/src/bhyve/bhyve_command.c
@@ -411,9 +411,7 @@ bhyveBuildGraphicsArgStr(const virDomainDef *def,
                     return -1;
                 graphics->data.vnc.port = port;
             } else {
-                if (virPortAllocatorSetUsed(driver->remotePorts,
-                                            graphics->data.vnc.port,
-                                            true) < 0)
+                if (virPortAllocatorSetUsed(graphics->data.vnc.port, true) < 0)
                     VIR_WARN("Failed to mark VNC port '%d' as used by '%s'",
                              graphics->data.vnc.port, def->name);
             }
diff --git a/src/bhyve/bhyve_process.c b/src/bhyve/bhyve_process.c
index 5076246..e215f6a 100644
--- a/src/bhyve/bhyve_process.c
+++ b/src/bhyve/bhyve_process.c
@@ -425,9 +425,7 @@ virBhyveProcessReconnect(virDomainObjPtr vm,
              if (vm->def->ngraphics == 1 &&
                  vm->def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC) {
                  int vnc_port = vm->def->graphics[0]->data.vnc.port;
-                 if (virPortAllocatorSetUsed(data->driver->remotePorts,
-                                             vnc_port,
-                                             true) < 0) {
+                 if (virPortAllocatorSetUsed(vnc_port, true) < 0) {
                      VIR_WARN("Failed to mark VNC port '%d' as used by '%s'",
                               vnc_port, vm->def->name);
                  }
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index a0f430f..b0f8ea8 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -4153,8 +4153,7 @@ qemuProcessStartHook(virQEMUDriverPtr driver,
 
 
 static int
-qemuProcessGraphicsReservePorts(virQEMUDriverPtr driver,
-                                virDomainGraphicsDefPtr graphics,
+qemuProcessGraphicsReservePorts(virDomainGraphicsDefPtr graphics,
                                 bool reconnect)
 {
     virDomainGraphicsListenDefPtr glisten;
@@ -4172,16 +4171,12 @@ qemuProcessGraphicsReservePorts(virQEMUDriverPtr driver,
     case VIR_DOMAIN_GRAPHICS_TYPE_VNC:
         if (!graphics->data.vnc.autoport ||
             reconnect) {
-            if (virPortAllocatorSetUsed(driver->remotePorts,
-                                        graphics->data.vnc.port,
-                                        true) < 0)
+            if (virPortAllocatorSetUsed(graphics->data.vnc.port, true) < 0)
                 return -1;
             graphics->data.vnc.portReserved = true;
         }
         if (graphics->data.vnc.websocket > 0 &&
-            virPortAllocatorSetUsed(driver->remotePorts,
-                                    graphics->data.vnc.websocket,
-                                    true) < 0)
+            virPortAllocatorSetUsed(graphics->data.vnc.websocket, true) < 0)
             return -1;
         break;
 
@@ -4190,17 +4185,13 @@ qemuProcessGraphicsReservePorts(virQEMUDriverPtr driver,
             return 0;
 
         if (graphics->data.spice.port > 0) {
-            if (virPortAllocatorSetUsed(driver->remotePorts,
-                                        graphics->data.spice.port,
-                                        true) < 0)
+            if (virPortAllocatorSetUsed(graphics->data.spice.port, true) < 0)
                 return -1;
             graphics->data.spice.portReserved = true;
         }
 
         if (graphics->data.spice.tlsPort > 0) {
-            if (virPortAllocatorSetUsed(driver->remotePorts,
-                                        graphics->data.spice.tlsPort,
-                                        true) < 0)
+            if (virPortAllocatorSetUsed(graphics->data.spice.tlsPort, true) < 0)
                 return -1;
             graphics->data.spice.tlsPortReserved = true;
         }
@@ -4390,7 +4381,7 @@ qemuProcessSetupGraphics(virQEMUDriverPtr driver,
         for (i = 0; i < vm->def->ngraphics; i++) {
             graphics = vm->def->graphics[i];
 
-            if (qemuProcessGraphicsReservePorts(driver, graphics, false) < 0)
+            if (qemuProcessGraphicsReservePorts(graphics, false) < 0)
                 goto cleanup;
         }
     }
@@ -6551,9 +6542,7 @@ void qemuProcessStop(virQEMUDriverPtr driver,
                 virPortAllocatorRelease(driver->remotePorts,
                                         graphics->data.vnc.port);
             } else if (graphics->data.vnc.portReserved) {
-                virPortAllocatorSetUsed(driver->remotePorts,
-                                        graphics->data.spice.port,
-                                        false);
+                virPortAllocatorSetUsed(graphics->data.spice.port, false);
                 graphics->data.vnc.portReserved = false;
             }
             if (graphics->data.vnc.websocketGenerated) {
@@ -6562,9 +6551,7 @@ void qemuProcessStop(virQEMUDriverPtr driver,
                 graphics->data.vnc.websocketGenerated = false;
                 graphics->data.vnc.websocket = -1;
             } else if (graphics->data.vnc.websocket) {
-                virPortAllocatorSetUsed(driver->remotePorts,
-                                        graphics->data.vnc.websocket,
-                                        false);
+                virPortAllocatorSetUsed(graphics->data.vnc.websocket, false);
             }
         }
         if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) {
@@ -6575,16 +6562,12 @@ void qemuProcessStop(virQEMUDriverPtr driver,
                                         graphics->data.spice.tlsPort);
             } else {
                 if (graphics->data.spice.portReserved) {
-                    virPortAllocatorSetUsed(driver->remotePorts,
-                                            graphics->data.spice.port,
-                                            false);
+                    virPortAllocatorSetUsed(graphics->data.spice.port, false);
                     graphics->data.spice.portReserved = false;
                 }
 
                 if (graphics->data.spice.tlsPortReserved) {
-                    virPortAllocatorSetUsed(driver->remotePorts,
-                                            graphics->data.spice.tlsPort,
-                                            false);
+                    virPortAllocatorSetUsed(graphics->data.spice.tlsPort, false);
                     graphics->data.spice.tlsPortReserved = false;
                 }
             }
@@ -7177,9 +7160,7 @@ qemuProcessReconnect(void *opaque)
     }
 
     for (i = 0; i < obj->def->ngraphics; i++) {
-        if (qemuProcessGraphicsReservePorts(driver,
-                                            obj->def->graphics[i],
-                                            true) < 0)
+        if (qemuProcessGraphicsReservePorts(obj->def->graphics[i], true) < 0)
             goto error;
     }
 
diff --git a/src/util/virportallocator.c b/src/util/virportallocator.c
index cd64356..76346c7 100644
--- a/src/util/virportallocator.c
+++ b/src/util/virportallocator.c
@@ -293,8 +293,7 @@ int virPortAllocatorRelease(virPortRangePtr range,
     return ret;
 }
 
-int virPortAllocatorSetUsed(virPortRangePtr range,
-                            unsigned short port,
+int virPortAllocatorSetUsed(unsigned short port,
                             bool value)
 {
     int ret = -1;
@@ -305,12 +304,6 @@ int virPortAllocatorSetUsed(virPortRangePtr range,
 
     virObjectLock(pa);
 
-    if (port < range->start ||
-        port > range->end) {
-        ret = 0;
-        goto cleanup;
-    }
-
     if (value) {
         if (virBitmapIsBitSet(pa->bitmap, port) ||
             virBitmapSetBit(pa->bitmap, port) < 0) {
diff --git a/src/util/virportallocator.h b/src/util/virportallocator.h
index e9b9038..8511eca 100644
--- a/src/util/virportallocator.h
+++ b/src/util/virportallocator.h
@@ -45,8 +45,6 @@ int virPortAllocatorAcquire(virPortRangePtr range,
 int virPortAllocatorRelease(virPortRangePtr range,
                             unsigned short port);
 
-int virPortAllocatorSetUsed(virPortRangePtr range,
-                            unsigned short port,
-                            bool value);
+int virPortAllocatorSetUsed(unsigned short port, bool value);
 
 #endif /* __VIR_PORT_ALLOCATOR_H__ */
-- 
1.8.3.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/6] port allocator: remove range on manual port reserving
Posted by Michal Privoznik 7 years, 3 months ago 
On 12/20/2017 07:35 AM, Nikolay Shirokovskiy wrote:
> Range check in virPortAllocatorSetUsed is not useful anymore
> when we manage ports for entire unsigned short range values.
> ---
>  src/bhyve/bhyve_command.c   |  4 +---
>  src/bhyve/bhyve_process.c   |  4 +---
>  src/qemu/qemu_process.c     | 41 +++++++++++------------------------------
>  src/util/virportallocator.c |  9 +--------
>  src/util/virportallocator.h |  4 +---
>  5 files changed, 15 insertions(+), 47 deletions(-)
> 

> diff --git a/src/util/virportallocator.c b/src/util/virportallocator.c
> index cd64356..76346c7 100644
> --- a/src/util/virportallocator.c
> +++ b/src/util/virportallocator.c
> @@ -293,8 +293,7 @@ int virPortAllocatorRelease(virPortRangePtr range,
>      return ret;
>  }
>  
> -int virPortAllocatorSetUsed(virPortRangePtr range,
> -                            unsigned short port,
> +int virPortAllocatorSetUsed(unsigned short port,
>                              bool value)
>  {
>      int ret = -1;
> @@ -305,12 +304,6 @@ int virPortAllocatorSetUsed(virPortRangePtr range,
>  
>      virObjectLock(pa);
>  
> -    if (port < range->start ||
> -        port > range->end) {
> -        ret = 0;
> -        goto cleanup;
> -    }
> -


1: ^^^

>      if (value) {
>          if (virBitmapIsBitSet(pa->bitmap, port) ||
>              virBitmapSetBit(pa->bitmap, port) < 0) {
> diff --git a/src/util/virportallocator.h b/src/util/virportallocator.h
> index e9b9038..8511eca 100644
> --- a/src/util/virportallocator.h
> +++ b/src/util/virportallocator.h
> @@ -45,8 +45,6 @@ int virPortAllocatorAcquire(virPortRangePtr range,
>  int virPortAllocatorRelease(virPortRangePtr range,
>                              unsigned short port);
>  
> -int virPortAllocatorSetUsed(virPortRangePtr range,
> -                            unsigned short port,
> -                            bool value);
> +int virPortAllocatorSetUsed(unsigned short port, bool value);

I'm not a big fan of this. Since virPortRange (or whatever name you'll
give it) is going to have a pointer to virPortAllocator object I rather
leave this signature as is. The idea (hunk 1) makes sense though

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/6] port allocator: remove range on manual port reserving
Posted by Nikolay Shirokovskiy 7 years, 3 months ago 

On 29.01.2018 09:09, Michal Privoznik wrote:
> On 12/20/2017 07:35 AM, Nikolay Shirokovskiy wrote:
>> Range check in virPortAllocatorSetUsed is not useful anymore
>> when we manage ports for entire unsigned short range values.
>> ---
>>  src/bhyve/bhyve_command.c   |  4 +---
>>  src/bhyve/bhyve_process.c   |  4 +---
>>  src/qemu/qemu_process.c     | 41 +++++++++++------------------------------
>>  src/util/virportallocator.c |  9 +--------
>>  src/util/virportallocator.h |  4 +---
>>  5 files changed, 15 insertions(+), 47 deletions(-)
>>
> 
>> diff --git a/src/util/virportallocator.c b/src/util/virportallocator.c
>> index cd64356..76346c7 100644
>> --- a/src/util/virportallocator.c
>> +++ b/src/util/virportallocator.c
>> @@ -293,8 +293,7 @@ int virPortAllocatorRelease(virPortRangePtr range,
>>      return ret;
>>  }
>>  
>> -int virPortAllocatorSetUsed(virPortRangePtr range,
>> -                            unsigned short port,
>> +int virPortAllocatorSetUsed(unsigned short port,
>>                              bool value)
>>  {
>>      int ret = -1;
>> @@ -305,12 +304,6 @@ int virPortAllocatorSetUsed(virPortRangePtr range,
>>  
>>      virObjectLock(pa);
>>  
>> -    if (port < range->start ||
>> -        port > range->end) {
>> -        ret = 0;
>> -        goto cleanup;
>> -    }
>> -
> 
> 
> 1: ^^^
> 
>>      if (value) {
>>          if (virBitmapIsBitSet(pa->bitmap, port) ||
>>              virBitmapSetBit(pa->bitmap, port) < 0) {
>> diff --git a/src/util/virportallocator.h b/src/util/virportallocator.h
>> index e9b9038..8511eca 100644
>> --- a/src/util/virportallocator.h
>> +++ b/src/util/virportallocator.h
>> @@ -45,8 +45,6 @@ int virPortAllocatorAcquire(virPortRangePtr range,
>>  int virPortAllocatorRelease(virPortRangePtr range,
>>                              unsigned short port);
>>  
>> -int virPortAllocatorSetUsed(virPortRangePtr range,
>> -                            unsigned short port,
>> -                            bool value);
>> +int virPortAllocatorSetUsed(unsigned short port, bool value);
> 
> I'm not a big fan of this. Since virPortRange (or whatever name you'll
> give it) is going to have a pointer to virPortAllocator object I rather
> leave this signature as is. The idea (hunk 1) makes sense though
> 

So this depends on how we get virPortAllocator object reference. Let's discuss later
after we come to agreement on previous patch.

Nikolay

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-8c64711

© 2016 - 2025 Red Hat, Inc.