Change the variable name to be a bit more descriptive and less confusing
when used with the data.network.actual->class_id.

Signed-off-by: John Ferlan <jferlan@redhat.com>
---
 src/conf/virnetworkobj.c    | 39 ++++++++++++++++++++-------------------
 src/conf/virnetworkobj.h    |  2 +-
 src/network/bridge_driver.c | 10 +++++-----
 3 files changed, 26 insertions(+), 25 deletions(-)

diff --git a/src/conf/virnetworkobj.c b/src/conf/virnetworkobj.c
index 533ed26..fb533b9 100644
--- a/src/conf/virnetworkobj.c
+++ b/src/conf/virnetworkobj.c
@@ -79,13 +79,13 @@ virNetworkObjNew(void)
     if (!(net = virObjectLockableNew(virNetworkObjClass)))
         return NULL;
 
-    if (!(net->class_id = virBitmapNew(CLASS_ID_BITMAP_SIZE)))
+    if (!(net->classIdMap = virBitmapNew(CLASS_ID_BITMAP_SIZE)))
         goto error;
 
     /* The first three class IDs are already taken */
-    ignore_value(virBitmapSetBit(net->class_id, 0));
-    ignore_value(virBitmapSetBit(net->class_id, 1));
-    ignore_value(virBitmapSetBit(net->class_id, 2));
+    ignore_value(virBitmapSetBit(net->classIdMap, 0));
+    ignore_value(virBitmapSetBit(net->classIdMap, 1));
+    ignore_value(virBitmapSetBit(net->classIdMap, 2));
 
     return net;
 
@@ -376,7 +376,7 @@ virNetworkObjDispose(void *obj)
 
     virNetworkDefFree(net->def);
     virNetworkDefFree(net->newDef);
-    virBitmapFree(net->class_id);
+    virBitmapFree(net->classIdMap);
     virObjectUnref(net->macmap);
 }
 
@@ -716,17 +716,17 @@ virNetworkObjFormat(virNetworkObjPtr net,
                     unsigned int flags)
 {
     virBuffer buf = VIR_BUFFER_INITIALIZER;
-    char *class_id = virBitmapFormat(net->class_id);
+    char *classIdStr = virBitmapFormat(net->classIdMap);
     size_t i;
 
-    if (!class_id)
+    if (!classIdStr)
         goto error;
 
     virBufferAddLit(&buf, "<networkstatus>\n");
     virBufferAdjustIndent(&buf, 2);
-    virBufferAsprintf(&buf, "<class_id bitmap='%s'/>\n", class_id);
+    virBufferAsprintf(&buf, "<class_id bitmap='%s'/>\n", classIdStr);
     virBufferAsprintf(&buf, "<floor sum='%llu'/>\n", net->floor_sum);
-    VIR_FREE(class_id);
+    VIR_FREE(classIdStr);
 
     for (i = 0; i < VIR_NETWORK_TAINT_LAST; i++) {
         if (net->taint & (1 << i))
@@ -783,7 +783,7 @@ virNetworkLoadState(virNetworkObjListPtr nets,
     xmlDocPtr xml = NULL;
     xmlNodePtr node = NULL, *nodes = NULL;
     xmlXPathContextPtr ctxt = NULL;
-    virBitmapPtr class_id_map = NULL;
+    virBitmapPtr classIdMap = NULL;
     unsigned long long floor_sum_val = 0;
     unsigned int taint = 0;
     int n;
@@ -820,18 +820,19 @@ virNetworkLoadState(virNetworkObjListPtr nets,
     if (xmlStrEqual(node->name, BAD_CAST "networkstatus")) {
         /* Newer network status file. Contains useful
          * info which are not to be found in bare config XML */
-        char *class_id = NULL;
+        char *classIdStr = NULL;
         char *floor_sum = NULL;
 
         ctxt->node = node;
-        if ((class_id = virXPathString("string(./class_id[1]/@bitmap)", ctxt))) {
-            if (virBitmapParse(class_id, &class_id_map,
+        if ((classIdStr = virXPathString("string(./class_id[1]/@bitmap)",
+                                         ctxt))) {
+            if (virBitmapParse(classIdStr, &classIdMap,
                                CLASS_ID_BITMAP_SIZE) < 0) {
-                VIR_FREE(class_id);
+                VIR_FREE(classIdStr);
                 goto error;
             }
         }
-        VIR_FREE(class_id);
+        VIR_FREE(classIdStr);
 
         floor_sum = virXPathString("string(./floor[1]/@sum)", ctxt);
         if (floor_sum &&
@@ -873,9 +874,9 @@ virNetworkLoadState(virNetworkObjListPtr nets,
     /* do not put any "goto error" below this comment */
 
     /* assign status data stored in the network object */
-    if (class_id_map) {
-        virBitmapFree(net->class_id);
-        net->class_id = class_id_map;
+    if (classIdMap) {
+        virBitmapFree(net->classIdMap);
+        net->classIdMap = classIdMap;
     }
 
     if (floor_sum_val > 0)
@@ -892,7 +893,7 @@ virNetworkLoadState(virNetworkObjListPtr nets,
 
  error:
     VIR_FREE(nodes);
-    virBitmapFree(class_id_map);
+    virBitmapFree(classIdMap);
     virNetworkDefFree(def);
     goto cleanup;
 }
diff --git a/src/conf/virnetworkobj.h b/src/conf/virnetworkobj.h
index a07c7bf..0192913 100644
--- a/src/conf/virnetworkobj.h
+++ b/src/conf/virnetworkobj.h
@@ -38,7 +38,7 @@ struct _virNetworkObj {
     virNetworkDefPtr def; /* The current definition */
     virNetworkDefPtr newDef; /* New definition to activate at shutdown */
 
-    virBitmapPtr class_id; /* bitmap of class IDs for QoS */
+    virBitmapPtr classIdMap; /* bitmap of class IDs for QoS */
     unsigned long long floor_sum; /* sum of all 'floor'-s of attached NICs */
 
     unsigned int taint;
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 5367773..ff0ad9c 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -5347,9 +5347,9 @@ networkNextClassID(virNetworkObjPtr obj)
 {
     ssize_t ret = 0;
 
-    ret = virBitmapNextClearBit(obj->class_id, -1);
+    ret = virBitmapNextClearBit(obj->classIdMap, -1);
 
-    if (ret < 0 || virBitmapSetBit(obj->class_id, ret) < 0)
+    if (ret < 0 || virBitmapSetBit(obj->classIdMap, ret) < 0)
         return -1;
 
     return ret;
@@ -5387,7 +5387,7 @@ networkPlugBandwidthImpl(virNetworkObjPtr obj,
     obj->floor_sum += ifaceBand->in->floor;
     /* update status file */
     if (virNetworkObjSaveStatus(driver->stateDir, obj) < 0) {
-        ignore_value(virBitmapClearBit(obj->class_id, class_id));
+        ignore_value(virBitmapClearBit(obj->classIdMap, class_id));
         obj->floor_sum -= ifaceBand->in->floor;
         iface->data.network.actual->class_id = 0;
         ignore_value(virNetDevBandwidthUnplug(obj->def->bridge, class_id));
@@ -5476,12 +5476,12 @@ networkUnplugBandwidth(virNetworkObjPtr obj,
         /* update sum of 'floor'-s of attached NICs */
         obj->floor_sum -= ifaceBand->in->floor;
         /* return class ID */
-        ignore_value(virBitmapClearBit(obj->class_id,
+        ignore_value(virBitmapClearBit(obj->classIdMap,
                                        iface->data.network.actual->class_id));
         /* update status file */
         if (virNetworkObjSaveStatus(driver->stateDir, obj) < 0) {
             obj->floor_sum += ifaceBand->in->floor;
-            ignore_value(virBitmapSetBit(obj->class_id,
+            ignore_value(virBitmapSetBit(obj->classIdMap,
                                          iface->data.network.actual->class_id));
             goto cleanup;
         }
-- 
2.9.4

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

On 07/26/2017 05:05 PM, John Ferlan wrote:
> Change the variable name to be a bit more descriptive and less confusing
> when used with the data.network.actual->class_id.
> 
> Signed-off-by: John Ferlan <jferlan@redhat.com>
> ---
>  src/conf/virnetworkobj.c    | 39 ++++++++++++++++++++-------------------
>  src/conf/virnetworkobj.h    |  2 +-
>  src/network/bridge_driver.c | 10 +++++-----
>  3 files changed, 26 insertions(+), 25 deletions(-)
> 
> diff --git a/src/conf/virnetworkobj.c b/src/conf/virnetworkobj.c
> index 533ed26..fb533b9 100644
> --- a/src/conf/virnetworkobj.c
> +++ b/src/conf/virnetworkobj.c
> @@ -79,13 +79,13 @@ virNetworkObjNew(void)
>      if (!(net = virObjectLockableNew(virNetworkObjClass)))
>          return NULL;
>  
> -    if (!(net->class_id = virBitmapNew(CLASS_ID_BITMAP_SIZE)))
> +    if (!(net->classIdMap = virBitmapNew(CLASS_ID_BITMAP_SIZE)))
>          goto error;

One thing I just realized: This creates a bitmap capable of holding
1<<16 bits - IOW it allocates 8KB of ram even though in reality just the
first 10-20 bits are going to be used. Back in the day when I was
implementing this there were no self-inflating bitmaps, but I guess we
do have them now, right? Maybe we can rewrite this (ofc after you merge
these to avoid conflicts) so that the new bitmap is used?

Michal

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

On 08/15/2017 11:32 AM, Michal Privoznik wrote:
> On 07/26/2017 05:05 PM, John Ferlan wrote:
>> Change the variable name to be a bit more descriptive and less confusing
>> when used with the data.network.actual->class_id.
>>
>> Signed-off-by: John Ferlan <jferlan@redhat.com>
>> ---
>>  src/conf/virnetworkobj.c    | 39 ++++++++++++++++++++-------------------
>>  src/conf/virnetworkobj.h    |  2 +-
>>  src/network/bridge_driver.c | 10 +++++-----
>>  3 files changed, 26 insertions(+), 25 deletions(-)
>>
>> diff --git a/src/conf/virnetworkobj.c b/src/conf/virnetworkobj.c
>> index 533ed26..fb533b9 100644
>> --- a/src/conf/virnetworkobj.c
>> +++ b/src/conf/virnetworkobj.c
>> @@ -79,13 +79,13 @@ virNetworkObjNew(void)
>>      if (!(net = virObjectLockableNew(virNetworkObjClass)))
>>          return NULL;
>>  
>> -    if (!(net->class_id = virBitmapNew(CLASS_ID_BITMAP_SIZE)))
>> +    if (!(net->classIdMap = virBitmapNew(CLASS_ID_BITMAP_SIZE)))
>>          goto error;
> 
> One thing I just realized: This creates a bitmap capable of holding
> 1<<16 bits - IOW it allocates 8KB of ram even though in reality just the
> first 10-20 bits are going to be used. Back in the day when I was
> implementing this there were no self-inflating bitmaps, but I guess we
> do have them now, right? Maybe we can rewrite this (ofc after you merge
> these to avoid conflicts) so that the new bitmap is used?
> 
> Michal
> 

I never gave it a second thought ;-) I haven't really dug into the
bitmap code, but I see virBitmapExpand, virBitmapSetBitExpand, and
virBitmapClearBitExpand that would seem to be useful for someone willing
to make that kind of alteration.

John

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

On 08/16/2017 03:09 AM, John Ferlan wrote:
> 
> 
> On 08/15/2017 11:32 AM, Michal Privoznik wrote:
>> On 07/26/2017 05:05 PM, John Ferlan wrote:
>>> Change the variable name to be a bit more descriptive and less confusing
>>> when used with the data.network.actual->class_id.
>>>
>>> Signed-off-by: John Ferlan <jferlan@redhat.com>
>>> ---
>>>  src/conf/virnetworkobj.c    | 39 ++++++++++++++++++++-------------------
>>>  src/conf/virnetworkobj.h    |  2 +-
>>>  src/network/bridge_driver.c | 10 +++++-----
>>>  3 files changed, 26 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/src/conf/virnetworkobj.c b/src/conf/virnetworkobj.c
>>> index 533ed26..fb533b9 100644
>>> --- a/src/conf/virnetworkobj.c
>>> +++ b/src/conf/virnetworkobj.c
>>> @@ -79,13 +79,13 @@ virNetworkObjNew(void)
>>>      if (!(net = virObjectLockableNew(virNetworkObjClass)))
>>>          return NULL;
>>>  
>>> -    if (!(net->class_id = virBitmapNew(CLASS_ID_BITMAP_SIZE)))
>>> +    if (!(net->classIdMap = virBitmapNew(CLASS_ID_BITMAP_SIZE)))
>>>          goto error;
>>
>> One thing I just realized: This creates a bitmap capable of holding
>> 1<<16 bits - IOW it allocates 8KB of ram even though in reality just the
>> first 10-20 bits are going to be used. Back in the day when I was
>> implementing this there were no self-inflating bitmaps, but I guess we
>> do have them now, right? Maybe we can rewrite this (ofc after you merge
>> these to avoid conflicts) so that the new bitmap is used?
>>
>> Michal
>>
> 
> I never gave it a second thought ;-) I haven't really dug into the
> bitmap code, but I see virBitmapExpand, virBitmapSetBitExpand, and
> virBitmapClearBitExpand that would seem to be useful for someone willing
> to make that kind of alteration.

Right. I'll start working on the patches and probably post them too.

Michal

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