Re: [PATCH v12 19/26] Introduce COLO mode and refactor relevant function

[Ian Jackson <Ian.Jackson@eu.citrix.com>]

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
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.

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.jackson@eu.citrix.com>

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