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

[Changlong Xie <xiecl.fnst@xxxxxxxxxxxxxx>]

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.jackson@xxxxxxxxxxxxx>
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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel