Cfr https://en.wikipedia.org/wiki/Spaghetti and https://en.wikipedia.org/wiki/Spaghetti_code! As today part of the team involved in the refactory review is on holiday I started looking at the average direction and state of this refactory. I looked at different angles like: 1- where we are; 2- what are actually doing; 3- what is the status of final code, looking at last commit in the branch. Honestly I'm not that happy on the status. 1- we just started the refactory. The reason is that code is quite old, there are plenty of dependency here and there, the code is mainly in a single file hard to understand full of small/medium/large functions. 2- actually we are trying to split this huge amount of code into separate files each handling a specific piece of functionality. The branch could be split in 3 pieces: 2.1- split into functionality files (actual one, about 150 patches) 2.2- remove global states; 2.3- encapsulate in gobject/glib. After 2.1 should be easier to understand where to edit a functionality and as dependencies should be less easier to add new ones or fix them. I repeat... SHOULD. If this were wrong after 2.3 code should not look that much as spaghetti code! Don't misunderstand me, the final code is better but IMHO far from what we want to probably achieve. I think that when a refactory is needed you should better understand the structure/features/objects which are involved in order to better define the relationship between them. I think this point is a bit missing. Let me make an example of what I mean. What a RedWorker should be supposed to do? What is it? IMHO a RedWorker should handle a thread that manage messages between display and cursor channel. Easy and simple definition but it comes with lot of implications: - thread informations are RedWorker stuff; - should deal (receive and send) messages dispatching to display and cursor channels. Once messages is dispatched the job is done! This means that RedWorker should not handle any message itself! - should not deal with drawing object as DisplayChannel will do it. This is the reason a lot of code is currently been moved from red_worker.c to display-channel.c/stream.c/others. But in the final commit there are still some fields which are strictly related to the display :( 3- oh my god! Try to open red_worker.c (and many other files) and see the FIXME, TODO and similar. Plenty of commented out code which were removed as too difficult to implement (and are working in the current master!). Well.. still to review/improve but surely not the code we want! The reason? Code is still in red_worker but would be better moved to other files. Note that separation was meant to be mostly completed after step 1 so the fact that after step 3 code is still there means that step 1 was not really completed. One way to check for dependency in C programs are to see: - function calls between modules (objdump or similar tools could be helpful); - include dependencies. Another thing to look are accessors functions (get/set). Let me show some example of what I found (take a look at final commit in refactory branch): - red_drawable_count in RedWorker. This field count drawables allocate in this worker. The field is used in red_drawable_unref and red_drawable_new (and some commented out code!). The RedDrawable structure is defined in red_parse_qxl.h. The life of this RedDrawable object is split between red_parse_qxl and red_worker creating a strong dependency between them. Note that red_parse_qxl mainly deal with reading messages from VM and managing structures at low level and has only an exception for this RedDrawable. The only thing is able to do is incrementing the reference counter while freeing and allocation is demanded to red_worker. Moving the refs to an upper layer structure would solve the issue. Another comment on this is why we have to keep Drawable and RedDrawable? Why don't DisplayChannel allocate a Drawable from RedDrawable and use it instead of dealing with two objects? - red_process_draw/destroy_primary_surface, mainly a bunch of calls to DisplayChannel - red_migrate_display, why still in red_worker? - red_worker_get_cursor_channel/red_worker_get_display_channel. These are called to finally initialize cursor/display channel allocated in red_worker_new. So red_worker_new partially initialize itself, then returns to red_dispatcher_init which is expected to finish the initialization of these structures. Note that part of the initialization is set some callback to some static functions defined in red_displatcher.c and strictly associated to cursor/display channel. Now just to demonstate how not that maintanable is that code try to add a function to destroy RedWorker and see how many files/functions you have to edit. I think that after a *_new function a a *_destroy *_unref could be called straight away. Actually this is not true. The reason why this should be true is that if some some reason initialization fails you should free the object. The reason why this is not currently true is that in some cases the *_new function allocate but not initialize entirely and another function is demanded to initliaze the object to a correct state (and at this point is possible to call *_destroy/*_unref). Frediano _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel