On 03/24/2016 11:45 PM, Ian Jackson wrote:
Changlong Xie writes ("[PATCH v12 19/26] Introduce COLO mode and refactor relevant
function"):
No functional changes.
Thanks, this is quite helpful.
Although:
+static char *qemu_disk_scsi_drive_string(libxl__gc *gc, const char *pdev_path,
+ int unit, const char *format,
+ const libxl_device_disk *disk,
+ int colo_mode)
+{
+ char *drive = NULL;
+
+ if (colo_mode == LIBXL__COLO_NONE)
+ {
+ drive = libxl__sprintf
+ (gc, "file=%s,if=scsi,bus=0,unit=%d,format=%s,cache=writeback",
+ pdev_path, unit, format);
+ return drive;
+ } else
+ abort();
Firstly, this has several style errors. The { after if should be on
the same line. And given that the if has { } so should the else.
+static char *qemu_disk_ide_drive_string(libxl__gc *gc, ...
And this function has a missing `else'.
Secondly, in the next patch you replace these ifs with switch(). It
would have been nicer to introduce the switch here in this patch, so
Will introduce "switch"
that the next patch doesn't contain pointless noise about the change
from if to switch.
I think it would be nice to improve this.
As a tip: if you want to change the intermediate state of the code in
the middle of a patch series, you can use the following sequence of
operations:
* git rebase -i
mark the patch whose afterwards-state needs to be changed `edit'
* when git-rebase stops, edit the tree so that the afterwards-state
is what you want it to be
* git add
* git commit (NOT git commit --amend)
You can use a commit message like
squash! Introduce COLO mode and refactor
v12: improve formatting
so that git rebase -i --autosquash will squash it
* git revert HEAD and edit the commit message to something like
SQUASH INTO NEXT COMMIT
* git rebase --continue
* git rebase -i --autosquash
find the "SQUASH INTO NEXT COMMIT" and mark the _next_ commit
"squash"
* git rebase will stop at the !squash and want you to edit
the combined commit message. Delete the !squash and move the
v12: to the right place
* git rebase will also stop and want you to edit the combined
commit message starting "SQUASH INTO NEXT COMMIT". Delete
"SQUASH INTO NEXT COMMIT" from the commit message. You can
leave the rest of the commit message unchanged.
Really usefully.
The result will be a series where all that has changed is the state
after your pre-patch; the end state of the tree is identical.
However, the style problems are abolished by the next patch, and the
next patch is now readable, so this is a relatively minor complaint.
Accordingly, even if you do not improve it as I suggeest:
Acked-by: Ian Jackson <ian.jack...@eu.citrix.com>
Thanks
-Xie
If you do improve things as I suggest above, without change the state
of the tree after subsequent patches, you may retain my ack.
Thanks,
Ian.
.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
