On 06/21/2010 03:37 PM, Eduardo Otubo wrote:
> All the comments from the previous email from Eric Blake are now fixed. Also
> fixed some styling by using indent on the whole file. Hope we can get this
> patch pushed to 0.8.2.
>
> Any additional comments are always welcome.
How hard would it be for you to separate styling changes from content
changes into two different commits? That is, it would make your patch a
little easier to review if the first stage were fixing indentation
issues in existing code, with no semantic changes, and the second change
is limited to semantic changes only, instead of the current mix of two
types of changes in one patch.
> - virBufferVSprintf(&buf, " -r mem --level lpar -F %s --filter lpar_ids=%d",
> + virBufferVSprintf(&buf,
> + " -r mem --level lpar -F %s --filter lpar_ids=%d",
> type ? "curr_mem" : "curr_max_mem", lpar_id);
For example, this was an indentation-only change.
> @@ -1744,24 +1747,28 @@ phypGetVIOSNextSlotNumber(virConnectPtr conn)
> virBuffer buf = VIR_BUFFER_INITIALIZER;
>
> if (!(profile = phypGetLparProfile(conn, vios_id))) {
> - VIR_ERROR("%s", "Unable to get VIOS profile name.");
> + VIR_ERROR0(_("Unable to get VIOS profile name."));
This is not an indentation change, but it is fixing a pre-existing area
of the code, so it would still fit better as a separate cleanup patch,
apart from the real meat.
> virBufferVSprintf(&buf, "-r prof --filter "
> "profile_names=%s -F virtual_eth_adapters,"
> "virtual_opti_pool_id,virtual_scsi_adapters,"
> "virtual_serial_adapters|sed -e 's/\"//g' -e "
> "'s/,/\\n/g'|sed -e 's/\\(^[0-9][0-9]\\*\\).*$/\\1/'"
> - "|sort|tail -n 1` +1 ))", profile);
> + "|sort|tail -n 1|sed -s 's/^[0]//'` +1 ))", profile);
Here's the first real meat I found in the patch (70 lines in!), but it
still has an issue - you are only stripping one, instead of all, leading
zeroes. I still think that doing the conversion to int, then the +1
operation, in C, will be much better than trying to do it in shell with
$(()).
My quick glance at spot checks in the rest of the file did see that you
are making progress; you did incorporate what looks like quite a few of
my comments. However, I did not complete my review this time around
because of the mix of multiple changes in one patch.
On the other hand, I agree that this still seems like something worth
getting in 0.8.2 if we can clean it up fast enough. Do you need any
help separating the patch into separate stages?
--
Eric Blake eblake@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list