Re: [libvirt] [PATCH 1/3] PHYP: Separating UUID functions in another file

On 11/19/2010 07:55 AM, Eduardo Otubo wrote: [I've rearranged my review a bit; .h before .c] ... While there are other counter-examples currently in libvirt.git, the general rule of thumb tends to be that .c files should include config.h first before any headers, and therefore .h files should not include it (because it will already have been included by the .c file including this .h). Where is virConnectPtr defined? This header should be self-contained, rather than relying on the .c file to include pre-requisite headers. Where is uuid_tablePtr defined? Did you mean int? Or should phypUUIDTable_Init be returning a uuid_tablePtr instead of an int? int id); remote host */ remote cmd */ Is this adequate? What's your rationale for this size? Typically a mac is 6 bytes, not 8. Is sign extension going to be a problem? s/int/size_t/ phyp_driver.c Why is this not in the phyp namespace? See earlier question about namespace. You should probably rename it in a separate patch prior to this one, so that the bulk of this patch will be just code motion with no renaming. In fact, when doing a refactor like this, splitting it into multiple steps makes it easier to review: + function renames and signature changes (for example, phypUUIDTable_WriteFile gaining an int type parameter), even though the new parameters have a constant value of DOMAIN + code motion into new files + add new code, by adding support for NET value alongside existing DOMAIN value of parameters added in first patch That way, the two real changes have a smaller line count, and the large line-count code motion change provably has 0 semantic change because it has no subtle differences introduced between the old and new location. That said, I'll go ahead and glance through the rest of this, but my review is very spotty because I'd like to see it broken into three commits before I do a thorough review (in fact, it looks like your static helper functions add NET support in this patch, but that you didn't ever pass NET to those functions until a later patch; my suggestion is therefore to refactor it to have the NET code in the static functions added at the same time or just before the code that uses it, but not during the code motion). Pre-existing code question: Why is the ./uuid_table file created with executable permissions? And why world-readable - is any process with a different euid ever expected to read it? ... Does configure.ac guarantee that this header exists if phyp is enabled, or are you lacking include guards? [answering myself] yes, configure.ac guarantees libssh2 for phyp, so no problem here. This is an internal file rather than a system header; it should be "domain_event.h". Generally, I prefer enums over static ints; also, I would have put DOMAIN as 0 and NET as 1 since NET is the new code and since NET sorts after DOMAIN. This repetition of file names is begging for a #DEFINE. Seems like a case for strdup() rather than the overhead of a virBuffer, if all the buffer will ever hold is a copy of a single other string. Likewise, virAsprintf seems like overkill when strdup() would do the job. No need to stat() a file if you are just going to follow it with fopen(); just open it, and fopen() will fail with the same reason as stat() would have failed if it can't be done. File modes should usually be treated as octal, not hex constants (0777 rather than 0x1ff). Why not just: unsigned int i = uuid_table->nlpars - 1; These don't overlap, you can use memcpy() for speed. -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org