| Message ID | 1458720400-4699-20-git-send-email-xiecl.fnst@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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@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. > > > . >
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c index 4aca38e..2226004 100644 --- a/tools/libxl/libxl_dm.c +++ b/tools/libxl/libxl_dm.c @@ -751,6 +751,45 @@ static int libxl__dm_runas_helper(libxl__gc *gc, const char *username) } } +/* colo mode */ +enum { + LIBXL__COLO_NONE = 0, +}; + +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(); +} + +static char *qemu_disk_ide_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 = GCSPRINTF + ("file=%s,if=ide,index=%d,media=disk,format=%s,cache=writeback", + pdev_path, unit, format); + return drive; + } + abort(); +} + static int libxl__build_device_model_args_new(libxl__gc *gc, const char *dm, int guest_domid, const libxl_domain_config *guest_config, @@ -1164,6 +1203,7 @@ static int libxl__build_device_model_args_new(libxl__gc *gc, const char *format = qemu_disk_format_string(disks[i].format); char *drive; const char *pdev_path; + int colo_mode; if (dev_number == -1) { LOG(WARN, "unable to determine"" disk number for %s", @@ -1208,10 +1248,13 @@ static int libxl__build_device_model_args_new(libxl__gc *gc, * For other disks we translate devices 0..3 into * hd[a-d] and ignore the rest. */ + + colo_mode = LIBXL__COLO_NONE; if (strncmp(disks[i].vdev, "sd", 2) == 0) { - drive = libxl__sprintf - (gc, "file=%s,if=scsi,bus=0,unit=%d,format=%s,readonly=%s,cache=writeback", - pdev_path, disk, format, disks[i].readwrite ? "off" : "on"); + drive = qemu_disk_scsi_drive_string(gc, pdev_path, disk, + format, + &disks[i], + colo_mode); } else if (strncmp(disks[i].vdev, "xvd", 3) == 0) { /* * Do not add any emulated disk when PV disk are @@ -1234,12 +1277,16 @@ static int libxl__build_device_model_args_new(libxl__gc *gc, LOG(ERROR, "qemu-xen doesn't support read-only IDE disk drivers"); return ERROR_INVAL; } - drive = libxl__sprintf - (gc, "file=%s,if=ide,index=%d,media=disk,format=%s,cache=writeback", - pdev_path, disk, format); + drive = qemu_disk_ide_drive_string(gc, pdev_path, disk, + format, + &disks[i], + colo_mode); } else { continue; /* Do not emulate this disk */ } + + if (!drive) + continue; } flexarray_append(dm_args, "-drive");