| Message ID | 1453375338-13508-2-git-send-email-rudyflyzhang@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/21/2016 04:22 AM, Rudy Zhang wrote: > Add hmp command for incremental backup in drive-backup. > It need a bitmap to backup data from drive-image to incremental image, > so before it need add bitmap for this device to track io. > Usage: > drive_backup [-n] [-f] device target [bitmap] [format] > > Signed-off-by: Rudy Zhang <rudyflyzhang@gmail.com> > --- > hmp-commands.hx | 5 +++-- > hmp.c | 16 ++++++++++++++-- > 2 files changed, 17 insertions(+), 4 deletions(-) > > diff --git a/hmp-commands.hx b/hmp-commands.hx > index bb52e4d..7378aaa 100644 > --- a/hmp-commands.hx > +++ b/hmp-commands.hx > @@ -1180,12 +1180,13 @@ ETEXI > > { > .name = "drive_backup", > - .args_type = "reuse:-n,full:-f,device:B,target:s,format:s?", > - .params = "[-n] [-f] device target [format]", > + .args_type = "reuse:-n,full:-f,device:B,target:s,bitmap:s?,format:s?", > + .params = "[-n] [-f] device target [bitmap] [format]", This is HMP, so it may not matter, but this is not backwards compatible. Scripts targetting the old style of passing a format will now have that format string interpreted as a bitmap name with no format. Better would be to stick [bitmap] at the end, not the middle. > @@ -1098,6 +1100,17 @@ void hmp_drive_backup(Monitor *mon, const QDict *qdict) > return; > } > > + if (full && bitmap) { > + error_setg(&err, "Parameter 'bitmap' if conflict with '-f'"); s/if conflict/conflicts/ > + hmp_handle_error(mon, &err); > + return; > + } else if (full) > + sync = MIRROR_SYNC_MODE_FULL; Needs {}. Run your patch through scripts/checkpatch.pl, to flag this and other style violations.
On 01/21/2016 11:39 AM, Eric Blake wrote: > On 01/21/2016 04:22 AM, Rudy Zhang wrote: >> Add hmp command for incremental backup in drive-backup. >> It need a bitmap to backup data from drive-image to incremental image, >> so before it need add bitmap for this device to track io. >> Usage: >> drive_backup [-n] [-f] device target [bitmap] [format] >> >> Signed-off-by: Rudy Zhang <rudyflyzhang@gmail.com> >> --- >> hmp-commands.hx | 5 +++-- >> hmp.c | 16 ++++++++++++++-- >> 2 files changed, 17 insertions(+), 4 deletions(-) >> >> diff --git a/hmp-commands.hx b/hmp-commands.hx >> index bb52e4d..7378aaa 100644 >> --- a/hmp-commands.hx >> +++ b/hmp-commands.hx >> @@ -1180,12 +1180,13 @@ ETEXI >> >> { >> .name = "drive_backup", >> - .args_type = "reuse:-n,full:-f,device:B,target:s,format:s?", >> - .params = "[-n] [-f] device target [format]", >> + .args_type = "reuse:-n,full:-f,device:B,target:s,bitmap:s?,format:s?", >> + .params = "[-n] [-f] device target [bitmap] [format]", > > This is HMP, so it may not matter, but this is not backwards compatible. > Scripts targetting the old style of passing a format will now have that > format string interpreted as a bitmap name with no format. Better would > be to stick [bitmap] at the end, not the middle. > I'd like to also point out that the method of intuiting the backup mode based on the parameters present won't expand very well as we add more modes, especially if there's ever an addition for a differential backup mode. Maybe it's fine because it's HMP and backwards compatibility is not a real concern ... > >> @@ -1098,6 +1100,17 @@ void hmp_drive_backup(Monitor *mon, const QDict *qdict) >> return; >> } >> >> + if (full && bitmap) { >> + error_setg(&err, "Parameter 'bitmap' if conflict with '-f'"); > > s/if conflict/conflicts/ > >> + hmp_handle_error(mon, &err); >> + return; >> + } else if (full) >> + sync = MIRROR_SYNC_MODE_FULL; > > Needs {}. Run your patch through scripts/checkpatch.pl, to flag this > and other style violations. >
On 16/1/22 ??12:39, Eric Blake wrote: > On 01/21/2016 04:22 AM, Rudy Zhang wrote: >> Add hmp command for incremental backup in drive-backup. >> It need a bitmap to backup data from drive-image to incremental image, >> so before it need add bitmap for this device to track io. >> Usage: >> drive_backup [-n] [-f] device target [bitmap] [format] >> >> Signed-off-by: Rudy Zhang <rudyflyzhang@gmail.com> >> --- >> hmp-commands.hx | 5 +++-- >> hmp.c | 16 ++++++++++++++-- >> 2 files changed, 17 insertions(+), 4 deletions(-) >> >> diff --git a/hmp-commands.hx b/hmp-commands.hx >> index bb52e4d..7378aaa 100644 >> --- a/hmp-commands.hx >> +++ b/hmp-commands.hx >> @@ -1180,12 +1180,13 @@ ETEXI >> >> { >> .name = "drive_backup", >> - .args_type = "reuse:-n,full:-f,device:B,target:s,format:s?", >> - .params = "[-n] [-f] device target [format]", >> + .args_type = "reuse:-n,full:-f,device:B,target:s,bitmap:s?,format:s?", >> + .params = "[-n] [-f] device target [bitmap] [format]", > This is HMP, so it may not matter, but this is not backwards compatible. > Scripts targetting the old style of passing a format will now have that > format string interpreted as a bitmap name with no format. Better would > be to stick [bitmap] at the end, not the middle. But I have a question: If I don't want to input a 'format', only use 'bitmap', it will let 'bitmap' as 'format', This problem how to do it. > >> @@ -1098,6 +1100,17 @@ void hmp_drive_backup(Monitor *mon, const QDict *qdict) >> return; >> } >> >> + if (full && bitmap) { >> + error_setg(&err, "Parameter 'bitmap' if conflict with '-f'"); > s/if conflict/conflicts/ oh?made a mistake. >> + hmp_handle_error(mon, &err); >> + return; >> + } else if (full) >> + sync = MIRROR_SYNC_MODE_FULL; > Needs {}. Run your patch through scripts/checkpatch.pl, to flag this > and other style violations. I have checked these patches?but I ignored these warnings.
On 01/21/2016 07:04 PM, ?? wrote: >>> - .args_type = "reuse:-n,full:-f,device:B,target:s,format:s?", >>> - .params = "[-n] [-f] device target [format]", >>> + .args_type = "reuse:-n,full:-f,device:B,target:s,bitmap:s?,format:s?", >>> + .params = "[-n] [-f] device target [bitmap] [format]", >> This is HMP, so it may not matter, but this is not backwards compatible. >> Scripts targetting the old style of passing a format will now have that >> format string interpreted as a bitmap name with no format. Better would >> be to stick [bitmap] at the end, not the middle. > > But I have a question: If I don't want to input a 'format', only use 'bitmap', > it will let 'bitmap' as 'format', This problem how to do it. Several solutions, some nicer than others, some more complicated than others. I don't have any strong preference, so much as pointing out the design space: 1. You don't. If you want to use 'bitmap', you MUST supply 'format' (supplying format is good anyways, as implicit formats have led to CVEs in the past). 1.a. Possible variation: Teach the command to allow empty '' format to be a synonym for an implicit format, so that it could look like: drive_backup device target '' /path/to/bitmap 2. You modify the HMP parser to accept optionally-named arguments, so that you can then supply later optional arguments by name without having to provide the earlier optional arguments. Maybe looking something like: drive_backup device target --bitmap=/path/to/bitmap 3. Instead of trying to overload an existing command, you create a new command. Particularly since your overload already declared that '-f' and 'bitmap' are incompatible. 4. maybe something else? >> Needs {}. Run your patch through scripts/checkpatch.pl, to flag this >> and other style violations. > > I have checked these patches?but I ignored these warnings. Not a good idea. And even in the rare case that you plan on ignoring the warnings, you should at least document in the commit message your justification for doing so.
diff --git a/hmp-commands.hx b/hmp-commands.hx index bb52e4d..7378aaa 100644 --- a/hmp-commands.hx +++ b/hmp-commands.hx @@ -1180,12 +1180,13 @@ ETEXI { .name = "drive_backup", - .args_type = "reuse:-n,full:-f,device:B,target:s,format:s?", - .params = "[-n] [-f] device target [format]", + .args_type = "reuse:-n,full:-f,device:B,target:s,bitmap:s?,format:s?", + .params = "[-n] [-f] device target [bitmap] [format]", .help = "initiates a point-in-time\n\t\t\t" "copy for a device. The device's contents are\n\t\t\t" "copied to the new image file, excluding data that\n\t\t\t" "is written after the command is started.\n\t\t\t" + "With bitmap will start incremental backup.\n\t\t\t" "The -n flag requests QEMU to reuse the image found\n\t\t\t" "in new-image-file, instead of recreating it from scratch.\n\t\t\t" "The -f flag requests QEMU to copy the whole disk,\n\t\t\t" diff --git a/hmp.c b/hmp.c index 54f2620..f8c33cd 100644 --- a/hmp.c +++ b/hmp.c @@ -1086,11 +1086,13 @@ void hmp_drive_backup(Monitor *mon, const QDict *qdict) { const char *device = qdict_get_str(qdict, "device"); const char *filename = qdict_get_str(qdict, "target"); + const char *bitmap = qdict_get_try_str(qdict, "bitmap"); const char *format = qdict_get_try_str(qdict, "format"); bool reuse = qdict_get_try_bool(qdict, "reuse", false); bool full = qdict_get_try_bool(qdict, "full", false); enum NewImageMode mode; Error *err = NULL; + enum MirrorSyncMode sync; if (!filename) { error_setg(&err, QERR_MISSING_PARAMETER, "target"); @@ -1098,6 +1100,17 @@ void hmp_drive_backup(Monitor *mon, const QDict *qdict) return; } + if (full && bitmap) { + error_setg(&err, "Parameter 'bitmap' if conflict with '-f'"); + hmp_handle_error(mon, &err); + return; + } else if (full) + sync = MIRROR_SYNC_MODE_FULL; + else if (bitmap) + sync = MIRROR_SYNC_MODE_INCREMENTAL; + else + sync = MIRROR_SYNC_MODE_TOP; + if (reuse) { mode = NEW_IMAGE_MODE_EXISTING; } else { @@ -1105,8 +1118,7 @@ void hmp_drive_backup(Monitor *mon, const QDict *qdict) } qmp_drive_backup(device, filename, !!format, format, - full ? MIRROR_SYNC_MODE_FULL : MIRROR_SYNC_MODE_TOP, - true, mode, false, 0, false, NULL, + sync, true, mode, false, 0, !!bitmap, bitmap, false, 0, false, 0, &err); hmp_handle_error(mon, &err); }
Add hmp command for incremental backup in drive-backup. It need a bitmap to backup data from drive-image to incremental image, so before it need add bitmap for this device to track io. Usage: drive_backup [-n] [-f] device target [bitmap] [format] Signed-off-by: Rudy Zhang <rudyflyzhang@gmail.com> --- hmp-commands.hx | 5 +++-- hmp.c | 16 ++++++++++++++-- 2 files changed, 17 insertions(+), 4 deletions(-)