On 04/30/2010 06:42 AM, Cole Robinson wrote:
> Other than that, the code generally looks like (except for the compiler and
> syntax-check warnings).
>
> - Cole
>
> (Here's the patch inline for the benefit of other reviewers)
In addition to Cole's comments, here's some style nits that 'make
syntax-check' won't pick up on...
>> +++ b/src/qemu/qemu_driver.c
>> @@ -4871,7 +4871,7 @@ static int qemudDomainSaveFlag(virDomainPtr dom, const char *path,
>> if (header.compressed == QEMUD_SAVE_FORMAT_RAW) {
>> const char *args[] = { "cat", NULL };
>> qemuDomainObjEnterMonitorWithDriver(driver, vm);
>> - rc = qemuMonitorMigrateToCommand(priv->mon, 1, args, path);
>> + rc = qemuMonitorMigrateToCommand(priv->mon, QEMU_MONITOR_MIGRATE_BACKGROUND, args, path);
Where possible, wrap lines to fit 80 columns. For example,
rc = qemuMonitorMigrateToCommand(priv->mon,
QEMU_MONITOR_MIGRATE_BACKGROUND,
args, path);
>> - unsigned long flags ATTRIBUTE_UNUSED,
>> + unsigned long flags ,
s/flags ,/flags,/
>> const char *dname ATTRIBUTE_UNUSED,
>> unsigned long resource)
>> {
>> int ret = -1;
>> xmlURIPtr uribits = NULL;
>> qemuDomainObjPrivatePtr priv = vm->privateData;
>> + int background_flags = 0;
Flags should generally be 'unsigned int', to make it less likely to
cause problems with inadvertent sign extension if we ever reach 32 flags.
>>
>> /* Issue the migrate command. */
>> if (STRPREFIX(uri, "tcp:") && !STRPREFIX(uri, "tcp://")) {
>> @@ -9684,7 +9685,13 @@ static int doNativeMigrate(struct qemud_driver *driver,
>> goto cleanup;
>> }
>>
>> - if (qemuMonitorMigrateToHost(priv->mon, 1, uribits->server, uribits->port) < 0) {
>> + if (flags & VIR_MIGRATE_NON_SHARED_DISK)
>> + background_flags |= QEMU_MONITOR_MIGRATE_NON_SHARED_DISK;
That TAB will be caught by 'make syntax-check'.
>> +++ b/src/qemu/qemu_monitor_text.c
>> @@ -1132,18 +1132,19 @@ static int qemuMonitorTextMigrate(qemuMonitorPtr mon,
>> char *info = NULL;
>> int ret = -1;
>> char *safedest = qemuMonitorEscapeArg(dest);
>> - const char *extra;
>> + //const char *extra;
No point leaving a dead comment; version-control is there to let us see
what extra was declared as before your patch changed it to:
>> + const char extra[25] = " ";
Why 25? That seems like a magic number, and it wastes space compared to
the maximum amount that you strcat() into it below.
>>
>> if (!safedest) {
>> virReportOOMError();
>> return -1;
>> }
>> -
>> - if (background)
>> - extra = "-d ";
>> - else
>> - extra = " ";
>> -
>> + if (background & QEMU_MONITOR_MIGRATE_BACKGROUND)
>> + strcat(extra," -d");
>> + if (background & QEMU_MONITOR_MIGRATE_NON_SHARED_DISK)
>> + strcat(extra," -b");
>> + if (background & QEMU_MONITOR_MIGRATE_NON_SHARED_INC)
>> + strcat(extra," -i");
>> if (virAsprintf(&cmd, "migrate %s\"%s\"", extra, safedest) < 0) {
That combination of strcat() and virAsprintf() looks bad. In general,
you should avoid strcat() (it often has overflow problems). And since
there is already allocation going on with virAsprintf, it seems like the
better way to write this would be to convert both strcat and virAsprintf
into multiple uses of the virBuffer* API, in order to build a single
malloc'd string incrementally.
--
Eric Blake eblake@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list