src/conf/virinterfaceobj.c | 26 +++++++---- src/conf/virinterfaceobj.h | 2 +- src/conf/virnodedeviceobj.c | 29 +++++++----- src/conf/virnodedeviceobj.h | 2 +- src/conf/virsecretobj.c | 58 +++++++++++------------- src/conf/virsecretobj.h | 2 +- src/conf/virstorageobj.c | 92 +++++++++++++++++++++++--------------- src/conf/virstorageobj.h | 2 +- src/node_device/node_device_hal.c | 16 ++++--- src/node_device/node_device_udev.c | 13 ++++-- src/secret/secret_driver.c | 15 ++++--- src/storage/storage_driver.c | 65 +++++++++++++++------------ src/test/test_driver.c | 78 +++++++++++++++++--------------- 13 files changed, 225 insertions(+), 175 deletions(-)
v1: https://www.redhat.com/archives/libvir-list/2018-March/msg01295.html
NB: This can wait until 4.2.0 is release, but I figured I'd post this
now just to put it on the radar and of course in hopes that someone
will look during the idle moment or two before the release.
Changes since v1:
Short story: Rework the processing of the code
Longer story:
In his review Erik noted that there's a "fire dance" when processing
the vir*Obj*Remove APIs of requiring a locked object upon entry, then
adding a reference to that object, unlocking the object, locking the
list to which it is contained, and then relocking the object.
So it took some time to think about it and during one lengthy meeting
today I had the aha moment that the *Remove API's could take the same
key (e.g., uuid or name) used to Add or Find the object and use it for
the Remove API. This allows the Remove API to not require a locked (and
reffed) object upon entry and perform the lock dance, remove the object,
and return just just a reffed object forcing the caller to know to Unref
object.
Instead, let's simplify things. The *Remove API can take the key, Find
the object in the list, remove it from the hash tables, and dispose of
the object. In essence the antecedent to the Add or AssignDef API's
taking a def, creating an object, and adding it the object to the hash
table(s). If there are two *Remove threads competing, one will win and
perform the removal, while the other will call *Remove, but won't find
the object in the hash table, and just return none the wiser.
And yes, I think this can also work for the Domain code, but it's going
to take a few patch series to get there as that code is not consistent
between consumers.
John Ferlan (9):
secret: Rework LoadAllConfigs
secret: Alter virSecretObjListRemove processing
interface: Alter virInterfaceObjListRemove processing
nodedev: Alter virNodeDeviceObjListRemove processing
conf: Clean up virStoragePoolObjLoad error processing
storage: Clean up storagePoolCreateXML error processing
test: Clean up testStoragePoolCreateXML error processing
conf: Move virStoragePoolObjRemove closer to AssignDef
storagepool: Alter virStoragePoolObjRemove processing
src/conf/virinterfaceobj.c | 26 +++++++----
src/conf/virinterfaceobj.h | 2 +-
src/conf/virnodedeviceobj.c | 29 +++++++-----
src/conf/virnodedeviceobj.h | 2 +-
src/conf/virsecretobj.c | 58 +++++++++++-------------
src/conf/virsecretobj.h | 2 +-
src/conf/virstorageobj.c | 92 +++++++++++++++++++++++---------------
src/conf/virstorageobj.h | 2 +-
src/node_device/node_device_hal.c | 16 ++++---
src/node_device/node_device_udev.c | 13 ++++--
src/secret/secret_driver.c | 15 ++++---
src/storage/storage_driver.c | 65 +++++++++++++++------------
src/test/test_driver.c | 78 +++++++++++++++++---------------
13 files changed, 225 insertions(+), 175 deletions(-)
--
2.13.6
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
ping? Tks - John On 03/28/2018 05:19 PM, John Ferlan wrote: > v1: https://www.redhat.com/archives/libvir-list/2018-March/msg01295.html > > NB: This can wait until 4.2.0 is release, but I figured I'd post this > now just to put it on the radar and of course in hopes that someone > will look during the idle moment or two before the release. > > Changes since v1: > > Short story: Rework the processing of the code > > Longer story: > > In his review Erik noted that there's a "fire dance" when processing > the vir*Obj*Remove APIs of requiring a locked object upon entry, then > adding a reference to that object, unlocking the object, locking the > list to which it is contained, and then relocking the object. > > So it took some time to think about it and during one lengthy meeting > today I had the aha moment that the *Remove API's could take the same > key (e.g., uuid or name) used to Add or Find the object and use it for > the Remove API. This allows the Remove API to not require a locked (and > reffed) object upon entry and perform the lock dance, remove the object, > and return just just a reffed object forcing the caller to know to Unref > object. > > Instead, let's simplify things. The *Remove API can take the key, Find > the object in the list, remove it from the hash tables, and dispose of > the object. In essence the antecedent to the Add or AssignDef API's > taking a def, creating an object, and adding it the object to the hash > table(s). If there are two *Remove threads competing, one will win and > perform the removal, while the other will call *Remove, but won't find > the object in the hash table, and just return none the wiser. > > And yes, I think this can also work for the Domain code, but it's going > to take a few patch series to get there as that code is not consistent > between consumers. > > John Ferlan (9): > secret: Rework LoadAllConfigs > secret: Alter virSecretObjListRemove processing > interface: Alter virInterfaceObjListRemove processing > nodedev: Alter virNodeDeviceObjListRemove processing > conf: Clean up virStoragePoolObjLoad error processing > storage: Clean up storagePoolCreateXML error processing > test: Clean up testStoragePoolCreateXML error processing > conf: Move virStoragePoolObjRemove closer to AssignDef > storagepool: Alter virStoragePoolObjRemove processing > > src/conf/virinterfaceobj.c | 26 +++++++---- > src/conf/virinterfaceobj.h | 2 +- > src/conf/virnodedeviceobj.c | 29 +++++++----- > src/conf/virnodedeviceobj.h | 2 +- > src/conf/virsecretobj.c | 58 +++++++++++------------- > src/conf/virsecretobj.h | 2 +- > src/conf/virstorageobj.c | 92 +++++++++++++++++++++++--------------- > src/conf/virstorageobj.h | 2 +- > src/node_device/node_device_hal.c | 16 ++++--- > src/node_device/node_device_udev.c | 13 ++++-- > src/secret/secret_driver.c | 15 ++++--- > src/storage/storage_driver.c | 65 +++++++++++++++------------ > src/test/test_driver.c | 78 +++++++++++++++++--------------- > 13 files changed, 225 insertions(+), 175 deletions(-) > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Mar 28, 2018 at 11:19 PM +0200, John Ferlan <jferlan@redhat.com> wrote: > v1: https://www.redhat.com/archives/libvir-list/2018-March/msg01295.html > > NB: This can wait until 4.2.0 is release, but I figured I'd post this > now just to put it on the radar and of course in hopes that someone > will look during the idle moment or two before the release. > > Changes since v1: > > Short story: Rework the processing of the code > > Longer story: > > In his review Erik noted that there's a "fire dance" when processing > the vir*Obj*Remove APIs of requiring a locked object upon entry, then > adding a reference to that object, unlocking the object, locking the > list to which it is contained, and then relocking the object. > > So it took some time to think about it and during one lengthy meeting > today I had the aha moment that the *Remove API's could take the same > key (e.g., uuid or name) used to Add or Find the object and use it for > the Remove API. This allows the Remove API to not require a locked (and > reffed) object upon entry and perform the lock dance, remove the object, > and return just just a reffed object forcing the caller to know to Unref > object. > > Instead, let's simplify things. The *Remove API can take the key, Find > the object in the list, remove it from the hash tables, and dispose of > the object. In essence the antecedent to the Add or AssignDef API's > taking a def, creating an object, and adding it the object to the hash > table(s). If there are two *Remove threads competing, one will win and > perform the removal, while the other will call *Remove, but won't find > the object in the hash table, and just return none the wiser. > > And yes, I think this can also work for the Domain code, but it's going > to take a few patch series to get there as that code is not consistent > between consumers. > > John Ferlan (9): > secret: Rework LoadAllConfigs > secret: Alter virSecretObjListRemove processing > interface: Alter virInterfaceObjListRemove processing > nodedev: Alter virNodeDeviceObjListRemove processing > conf: Clean up virStoragePoolObjLoad error processing > storage: Clean up storagePoolCreateXML error processing > test: Clean up testStoragePoolCreateXML error processing > conf: Move virStoragePoolObjRemove closer to AssignDef > storagepool: Alter virStoragePoolObjRemove processing Side note: Wouldn’t is be useful to refactor all the vir*ObjList* things as they’re looking quite similar? Not sure if it’s easily feasible in all places. […snip] -- Beste Grüße / Kind regards Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 04/10/2018 04:47 AM, Marc Hartmayer wrote: > On Wed, Mar 28, 2018 at 11:19 PM +0200, John Ferlan <jferlan@redhat.com> wrote: >> v1: https://www.redhat.com/archives/libvir-list/2018-March/msg01295.html >> >> NB: This can wait until 4.2.0 is release, but I figured I'd post this >> now just to put it on the radar and of course in hopes that someone >> will look during the idle moment or two before the release. >> >> Changes since v1: >> >> Short story: Rework the processing of the code >> >> Longer story: >> >> In his review Erik noted that there's a "fire dance" when processing >> the vir*Obj*Remove APIs of requiring a locked object upon entry, then >> adding a reference to that object, unlocking the object, locking the >> list to which it is contained, and then relocking the object. >> >> So it took some time to think about it and during one lengthy meeting >> today I had the aha moment that the *Remove API's could take the same >> key (e.g., uuid or name) used to Add or Find the object and use it for >> the Remove API. This allows the Remove API to not require a locked (and >> reffed) object upon entry and perform the lock dance, remove the object, >> and return just just a reffed object forcing the caller to know to Unref >> object. >> >> Instead, let's simplify things. The *Remove API can take the key, Find >> the object in the list, remove it from the hash tables, and dispose of >> the object. In essence the antecedent to the Add or AssignDef API's >> taking a def, creating an object, and adding it the object to the hash >> table(s). If there are two *Remove threads competing, one will win and >> perform the removal, while the other will call *Remove, but won't find >> the object in the hash table, and just return none the wiser. >> >> And yes, I think this can also work for the Domain code, but it's going >> to take a few patch series to get there as that code is not consistent >> between consumers. >> >> John Ferlan (9): >> secret: Rework LoadAllConfigs >> secret: Alter virSecretObjListRemove processing >> interface: Alter virInterfaceObjListRemove processing >> nodedev: Alter virNodeDeviceObjListRemove processing >> conf: Clean up virStoragePoolObjLoad error processing >> storage: Clean up storagePoolCreateXML error processing >> test: Clean up testStoragePoolCreateXML error processing >> conf: Move virStoragePoolObjRemove closer to AssignDef >> storagepool: Alter virStoragePoolObjRemove processing > > Side note: > Wouldn’t is be useful to refactor all the vir*ObjList* things as they’re > looking quite similar? Not sure if it’s easily feasible in all places. > Well - that was the point of what I started last year, but there hasn't been any general agreement or acceptance of patches for that. My changes made use of objects and more generic naming to unify things; however, they weren't acceptable with (IIRC and in my words) the preference being more specific naming using switch/case statements and shim API's. The last full posting (a v5) of what I had done is here: https://www.redhat.com/archives/libvir-list/2017-August/msg00659.html If you feel so inclined you can follow the history of comments through v4: https://www.redhat.com/archives/libvir-list/2017-August/msg00537.html and v3: https://www.redhat.com/archives/libvir-list/2017-June/msg00916.html w/ review comments starting here: https://www.redhat.com/archives/libvir-list/2017-July/msg01032.html Maybe once the domain code is modified to be more common (in process now) and if nwfilter ever could gain acceptance, something could be done. Still I have my doubts it'll happen especially since nwfilter patches just cannot get any sort of agreement, last try here: https://www.redhat.com/archives/libvir-list/2018-February/msg00325.html John > […snip] > > -- > Beste Grüße / Kind regards > Marc Hartmayer > > IBM Deutschland Research & Development GmbH > Vorsitzende des Aufsichtsrats: Martina Koederitz > Geschäftsführung: Dirk Wittkopp > Sitz der Gesellschaft: Böblingen > Registergericht: Amtsgericht Stuttgart, HRB 243294 > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2024 Red Hat, Inc.