package daemon: De/-activate all changed packages together

* We first process the node monitoring events, collecting the required
  package activation changes, then apply all changes together.
* Change the PackageFSActivationChangeItem/-Request structs. The former
  is no longer variable in size, which makes it easier to work with.

headers/private/package/packagefs.h

@@ -79,7 +79,9 @@ struct PackageFSActivationChangeItem {
 	uint32							nameLength;
 	dev_t							parentDeviceID;
 	ino_t							parentDirectoryID;
-	char							name[1];
+	char*							name;
+										// must point to a location within the
+										// request
 };
 
 struct PackageFSActivationChangeRequest {

src/add-ons/kernel/file_systems/packagefs/Volume.cpp

@@ -199,49 +199,6 @@ private:
 
 
 struct Volume::ActivationChangeRequest {
-public:
-	struct Iterator {
-		Iterator()
-			:
-			fRequest(NULL),
-			fNextItem(NULL),
-			fNextIndex(0)
-		{
-		}
-
-		Iterator(PackageFSActivationChangeRequest* request)
-			:
-			fRequest(request),
-			fNextItem(NULL),
-			fNextIndex(0)
-		{
-			if (fRequest != NULL && fRequest->itemCount > 0)
-				fNextItem = fRequest->items;
-		}
-
-		bool HasNext() const
-		{
-			return fNextItem != NULL;
-		}
-
-		PackageFSActivationChangeItem* Next()
-		{
-			if (fNextItem == NULL)
-				return NULL;
-
-			PackageFSActivationChangeItem* item = fNextItem;
-			fNextIndex++;
-			fNextItem = fNextIndex < fRequest->itemCount
-				? _NextItem(fNextItem) : NULL;
-			return item;
-		}
-
-	private:
-		PackageFSActivationChangeRequest*	fRequest;
-		PackageFSActivationChangeItem*		fNextItem;
-		uint32								fNextIndex;
-	};
-
 public:
 	ActivationChangeRequest()
 		:
@@ -270,22 +227,21 @@ public:
 		if (error != B_OK)
 			RETURN_ERROR(error);
 
-		// check the validity of the items
+		uint32 itemCount = fRequest->itemCount;
-		addr_t requestEnd = (addr_t)fRequest + fRequestSize;
+		const char* requestEnd = (const char*)fRequest + requestSize;
-		uint32 itemCount = 0;
+		if (&fRequest->items[itemCount] > (void*)requestEnd)
-		PackageFSActivationChangeItem* item = fRequest->items;
+			RETURN_ERROR(B_BAD_VALUE);
-		while (itemCount < fRequest->itemCount) {
-			if ((addr_t)item + sizeof(PackageFSActivationChangeItem)
-					> requestEnd
-				|| item->nameLength > B_FILE_NAME_LENGTH
-				|| (addr_t)item->name + item->nameLength > requestEnd
-				|| item->name[item->nameLength] != '\0'
-				|| strlen(item->name) != item->nameLength) {
-				RETURN_ERROR(B_BAD_VALUE);
-			}
 
-			itemCount++;
+		// adjust the item name pointers and check their validity
-			item = _NextItem(item);
+		addr_t nameDelta = (addr_t)fRequest - (addr_t)userRequest;
+		for (uint32 i = 0; i < itemCount; i++) {
+			PackageFSActivationChangeItem& item = fRequest->items[i];
+			item.name += nameDelta;
+			if (item.name < (char*)fRequest || item.name >= requestEnd)
+				RETURN_ERROR(B_BAD_VALUE);
+			size_t maxNameSize = requestEnd - item.name;
+			if (strnlen(item.name, maxNameSize) == maxNameSize)
+				RETURN_ERROR(B_BAD_VALUE);
 		}
 
 		return B_OK;
@@ -296,21 +252,9 @@ public:
 		return fRequest->itemCount;
 	}
 
-	Iterator GetIterator() const
+	PackageFSActivationChangeItem* ItemAt(uint32 index) const
 	{
-		return Iterator(fRequest);
+		return index < CountItems() ? &fRequest->items[index] : NULL;
-	}
-
-private:
-	friend class Iterator;
-		// for GCC 2
-
-private:
-	static inline PackageFSActivationChangeItem* _NextItem(
-		PackageFSActivationChangeItem* item)
-	{
-		return (PackageFSActivationChangeItem*)_ALIGN(
-			(addr_t)item->name + item->nameLength);
 	}
 
 private:
@@ -1362,7 +1306,8 @@ Volume::_LoadPackage(const char* name, Package*& _package)
 status_t
 Volume::_ChangeActivation(ActivationChangeRequest& request)
 {
-	if (request.CountItems() == 0)
+	uint32 itemCount = request.CountItems();
+	if (itemCount == 0)
 		return B_OK;
 
 	// first check the request
@@ -1371,9 +1316,8 @@ Volume::_ChangeActivation(ActivationChangeRequest& request)
 	{
 		VolumeReadLocker volumeLocker(this);
 
-		for (ActivationChangeRequest::Iterator it = request.GetIterator();
+		for (uint32 i = 0; i < itemCount; i++) {
-			it.HasNext();) {
+			PackageFSActivationChangeItem* item = request.ItemAt(i);
-			PackageFSActivationChangeItem* item = it.Next();
 			if (item->parentDeviceID != fPackagesDirectory->DeviceID()
 				|| item->parentDirectoryID != fPackagesDirectory->NodeID()) {
 				ERROR("Volume::_ChangeActivation(): mismatching packages "
@@ -1429,9 +1373,8 @@ INFORM("Volume::_ChangeActivation(): %" B_PRId32 " new packages, %" B_PRId32 " o
 
 	// load all new packages
 	int32 newPackageIndex = 0;
-	for (ActivationChangeRequest::Iterator it = request.GetIterator();
+	for (uint32 i = 0; i < itemCount; i++) {
-		it.HasNext();) {
+		PackageFSActivationChangeItem* item = request.ItemAt(i);
-		PackageFSActivationChangeItem* item = it.Next();
 
 		if (item->type != PACKAGE_FS_ACTIVATE_PACKAGE
 			&& item->type != PACKAGE_FS_REACTIVATE_PACKAGE) {
@@ -1457,9 +1400,8 @@ INFORM("Volume::_ChangeActivation(): %" B_PRId32 " new packages, %" B_PRId32 " o
 
 	// remove the old packages
 	int32 oldPackageIndex = 0;
-	for (ActivationChangeRequest::Iterator it = request.GetIterator();
+	for (uint32 i = 0; i < itemCount; i++) {
-		it.HasNext();) {
+		PackageFSActivationChangeItem* item = request.ItemAt(i);
-		PackageFSActivationChangeItem* item = it.Next();
 
 		if (item->type != PACKAGE_FS_DEACTIVATE_PACKAGE
 			&& item->type != PACKAGE_FS_REACTIVATE_PACKAGE) {

src/servers/package/Root.cpp

@@ -220,6 +220,9 @@ void
 Root::_ProcessNodeMonitorEvents(Volume* volume)
 {
 	volume->ProcessPendingNodeMonitorEvents();
+
+	if (volume->HasPendingPackageActivationChanges())
+		volume->ProcessPendingPackageActivationChanges();
 }
 
 

src/servers/package/Volume.cpp

@@ -81,7 +81,9 @@ Volume::Volume(BLooper* looper)
 	fPackagesByFileName(),
 	fPackagesByNodeRef(),
 	fPendingNodeMonitorEventsLock("pending node monitor events"),
-	fPendingNodeMonitorEvents()
+	fPendingNodeMonitorEvents(),
+	fPackagesToBeActivated(),
+	fPackagesToBeDeactivated()
 {
 	looper->AddHandler(this);
 }
@@ -275,7 +277,6 @@ Volume::ProcessPendingNodeMonitorEvents()
 	}
 
 	// process them
-// TODO: Don't do that individually.
 	while (NodeMonitorEvent* event = events.RemoveHead()) {
 		ObjectDeleter<NodeMonitorEvent> eventDeleter(event);
 		if (event->WasCreated())
@@ -286,6 +287,100 @@ Volume::ProcessPendingNodeMonitorEvents()
 }
 
 
+bool
+Volume::HasPendingPackageActivationChanges() const
+{
+	return !fPackagesToBeActivated.empty() || !fPackagesToBeDeactivated.empty();
+}
+
+
+void
+Volume::ProcessPendingPackageActivationChanges()
+{
+	if (!HasPendingPackageActivationChanges())
+		return;
+INFORM("Volume::ProcessPendingPackageActivationChanges(): activating %zu, deactivating %zu packages\n",
+fPackagesToBeActivated.size(), fPackagesToBeDeactivated.size());
+
+	// compute the size of the allocation we need for the activation change
+	// request
+	int32 itemCount
+		= fPackagesToBeActivated.size() + fPackagesToBeDeactivated.size();
+	size_t requestSize = sizeof(PackageFSActivationChangeRequest)
+		+ itemCount * sizeof(PackageFSActivationChangeItem);
+
+	for (PackageSet::iterator it = fPackagesToBeActivated.begin();
+		 it != fPackagesToBeActivated.end(); ++it) {
+		requestSize += (*it)->FileName().Length() + 1;
+	}
+
+	for (PackageSet::iterator it = fPackagesToBeDeactivated.begin();
+		 it != fPackagesToBeDeactivated.end(); ++it) {
+		requestSize += (*it)->FileName().Length() + 1;
+	}
+
+	// allocate and prepare the request
+	PackageFSActivationChangeRequest* request
+		= (PackageFSActivationChangeRequest*)malloc(requestSize);
+	if (request == NULL) {
+		ERROR("out of memory\n");
+		return;
+	}
+	MemoryDeleter requestDeleter(request);
+
+	request->itemCount = itemCount;
+
+	PackageFSActivationChangeItem* item = &request->items[0];
+	char* nameBuffer = (char*)(item + itemCount);
+
+	for (PackageSet::iterator it = fPackagesToBeActivated.begin();
+		it != fPackagesToBeActivated.end(); ++it, item++) {
+		_FillInActivationChangeItem(item, PACKAGE_FS_ACTIVATE_PACKAGE, *it,
+			nameBuffer);
+	}
+
+	for (PackageSet::iterator it = fPackagesToBeDeactivated.begin();
+		it != fPackagesToBeDeactivated.end(); ++it, item++) {
+		_FillInActivationChangeItem(item, PACKAGE_FS_DEACTIVATE_PACKAGE, *it,
+			nameBuffer);
+	}
+
+	// issue the request
+	int fd = OpenRootDirectory();
+	if (fd < 0) {
+		ERROR("Volume::ProcessPendingPackageActivationChanges(): failed to "
+			"open root directory: %s", strerror(fd));
+		return;
+	}
+	FileDescriptorCloser fdCloser(fd);
+
+	if (ioctl(fd, PACKAGE_FS_OPERATION_CHANGE_ACTIVATION, request, requestSize)
+			!= 0) {
+// TODO: We need more error information and error handling!
+		ERROR("Volume::ProcessPendingPackageActivationChanges(): failed to "
+			"activate packages: %s\n", strerror(errno));
+		return;
+	}
+
+	// Update our state, i.e. remove deactivated packages and mark activated
+	// packages accordingly.
+	for (PackageSet::iterator it = fPackagesToBeActivated.begin();
+		it != fPackagesToBeActivated.end(); ++it) {
+		(*it)->SetActive(true);
+	}
+
+	for (PackageSet::iterator it = fPackagesToBeDeactivated.begin();
+		it != fPackagesToBeDeactivated.end(); ++it) {
+		Package* package = *it;
+		_RemovePackage(package);
+		delete package;
+	}
+
+	fPackagesToBeActivated.clear();
+	fPackagesToBeDeactivated.clear();
+}
+
+
 void
 Volume::_HandleEntryCreatedOrRemoved(const BMessage* message, bool created)
 {
@@ -371,13 +466,13 @@ INFORM("Volume::_PackagesEntryCreated(\"%s\")\n", name);
 	entry.directory = fPackagesDirectoryRef.node;
 	status_t error = entry.set_name(name);
 	if (error != B_OK) {
-		ERROR("out of memory");
+		ERROR("out of memory\n");
 		return;
 	}
 
 	Package* package = new(std::nothrow) Package;
 	if (package == NULL) {
-		ERROR("out of memory");
+		ERROR("out of memory\n");
 		return;
 	}
 	ObjectDeleter<Package> packageDeleter(package);
@@ -392,46 +487,12 @@ INFORM("Volume::_PackagesEntryCreated(\"%s\")\n", name);
 	fPackagesByNodeRef.Insert(package);
 	packageDeleter.Detach();
 
-	// activate package
+	try {
-// TODO: Don't do that here!
+		fPackagesToBeActivated.insert(package);
-	size_t nameLength = strlen(package->FileName());
+	} catch (std::bad_alloc& exception) {
-	size_t requestSize = sizeof(PackageFSActivationChangeRequest) + nameLength;
+		ERROR("out of memory\n");
-	PackageFSActivationChangeRequest* request
-		= (PackageFSActivationChangeRequest*)malloc(requestSize);
-	if (request == NULL) {
-		ERROR("out of memory");
 		return;
 	}
-	MemoryDeleter requestDeleter(request);
-
-	request->itemCount = 1;
-	PackageFSActivationChangeItem& item = request->items[0];
-	item.type = PACKAGE_FS_ACTIVATE_PACKAGE;
-
-	item.packageDeviceID = package->NodeRef().device;
-	item.packageNodeID = package->NodeRef().node;
-
-	item.nameLength = nameLength;
-	item.parentDeviceID = fPackagesDirectoryRef.device;
-	item.parentDirectoryID = fPackagesDirectoryRef.node;
-	strcpy(item.name, package->FileName());
-
-	int fd = OpenRootDirectory();
-	if (fd < 0) {
-		ERROR("Volume::_PackagesEntryCreated(): failed to open root directory: "
-			"%s", strerror(fd));
-		return;
-	}
-	FileDescriptorCloser fdCloser(fd);
-
-	if (ioctl(fd, PACKAGE_FS_OPERATION_CHANGE_ACTIVATION, request, requestSize)
-			!= 0) {
-		ERROR("Volume::_PackagesEntryCreated(): activate packages: %s\n",
-			strerror(errno));
-		return;
-	}
-
-	package->SetActive(true);
 }
 
 
@@ -443,51 +504,50 @@ INFORM("Volume::_PackagesEntryRemoved(\"%s\")\n", name);
 	if (package == NULL)
 		return;
 
-	if (package->IsActive()) {
+	// Remove the package from the packages-to-be-activated set, if it is in
-		// deactivate the package
+	// there (unlikely, unless we see a create-remove-create sequence).
-// TODO: Don't do that here!
+	PackageSet::iterator it = fPackagesToBeActivated.find(package);
-		size_t nameLength = strlen(package->FileName());
+	if (it != fPackagesToBeActivated.end())
-		size_t requestSize = sizeof(PackageFSActivationChangeRequest)
+		fPackagesToBeActivated.erase(it);
-			+ nameLength;
-		PackageFSActivationChangeRequest* request
-			= (PackageFSActivationChangeRequest*)malloc(requestSize);
-		if (request == NULL) {
-			ERROR("out of memory");
-			return;
-		}
-		MemoryDeleter requestDeleter(request);
 
-		request->itemCount = 1;
+	// If the package isn't active, just remove it for good.
-		PackageFSActivationChangeItem& item = request->items[0];
+	if (!package->IsActive()) {
-		item.type = PACKAGE_FS_DEACTIVATE_PACKAGE;
+		_RemovePackage(package);
-
+		delete package;
-		item.packageDeviceID = package->NodeRef().device;
+		return;
-		item.packageNodeID = package->NodeRef().node;
-
-		item.nameLength = nameLength;
-		item.parentDeviceID = fPackagesDirectoryRef.device;
-		item.parentDirectoryID = fPackagesDirectoryRef.node;
-		strcpy(item.name, package->FileName());
-
-		int fd = OpenRootDirectory();
-		if (fd < 0) {
-			ERROR("Volume::_PackagesEntryRemoved(): failed to open root "
-				"directory: %s", strerror(fd));
-			return;
-		}
-		FileDescriptorCloser fdCloser(fd);
-
-		if (ioctl(fd, PACKAGE_FS_OPERATION_CHANGE_ACTIVATION, request,
-				requestSize) != 0) {
-			ERROR("Volume::_PackagesEntryRemoved(): activate packages: %s\n",
-				strerror(errno));
-			return;
-		}
 	}
 
+	// The package must be deactivated.
+	try {
+		fPackagesToBeDeactivated.insert(package);
+	} catch (std::bad_alloc& exception) {
+		ERROR("out of memory\n");
+		return;
+	}
+}
+
+
+void
+Volume::_FillInActivationChangeItem(PackageFSActivationChangeItem* item,
+	PackageFSActivationChangeType type, Package* package, char*& nameBuffer)
+{
+	item->type = type;
+	item->packageDeviceID = package->NodeRef().device;
+	item->packageNodeID = package->NodeRef().node;
+	item->nameLength = package->FileName().Length();
+	item->parentDeviceID = fPackagesDirectoryRef.device;
+	item->parentDirectoryID = fPackagesDirectoryRef.node;
+	item->name = nameBuffer;
+	strcpy(nameBuffer, package->FileName());
+	nameBuffer += package->FileName().Length() + 1;
+}
+
+
+void
+Volume::_RemovePackage(Package* package)
+{
 	fPackagesByFileName.Remove(package);
 	fPackagesByNodeRef.Remove(package);
-	delete package;
 }
 
 

src/servers/package/Volume.h

@@ -9,6 +9,8 @@
 #define VOLUME_H
 
 
+#include <set>
+
 #include <Handler.h>
 #include <Locker.h>
 #include <String.h>
@@ -68,10 +70,15 @@ public:
 
 			void				ProcessPendingNodeMonitorEvents();
 
+			bool				HasPendingPackageActivationChanges() const;
+			void				ProcessPendingPackageActivationChanges();
+
 private:
 			struct NodeMonitorEvent;
 			typedef DoublyLinkedList<NodeMonitorEvent> NodeMonitorEventList;
 
+			typedef std::set<Package*> PackageSet;
+
 private:
 			void				_HandleEntryCreatedOrRemoved(
 									const BMessage* message, bool created);
@@ -82,6 +89,12 @@ private:
 			void				_PackagesEntryCreated(const char* name);
 			void				_PackagesEntryRemoved(const char* name);
 
+			void				_FillInActivationChangeItem(
+									PackageFSActivationChangeItem* item,
+									PackageFSActivationChangeType type,
+									Package* package, char*& nameBuffer);
+			void				_RemovePackage(Package* package);
+
 			status_t			_ReadPackagesDirectory();
 			status_t			_GetActivePackages(int fd);
 
@@ -96,6 +109,8 @@ private:
 			PackageNodeRefHashTable fPackagesByNodeRef;
 			BLocker				fPendingNodeMonitorEventsLock;
 			NodeMonitorEventList fPendingNodeMonitorEvents;
+			PackageSet			fPackagesToBeActivated;
+			PackageSet			fPackagesToBeDeactivated;
 };