[Libvir] Re: [PATCH 7/7] Add KVM restore support using migration.

[Daniel Veillard]

On Sun, Aug 12, 2007 at 07:11:39PM -0400, Jim Paris wrote:
> 
> Signed-off-by: Jim Paris <jim at jtan.com>

> +    if ((xml = (char *)malloc(header.xml_len + 1)) == NULL) {
> +        qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED,
> +                         "out of memory");
> +        close(fd);
> +        return -1;
> +    }
> +
> +    if (read(fd, xml, header.xml_len) != header.xml_len) {
> +        qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED,
> +                         "failed to read XML");
> +        close(fd);
> +        free(xml);
> +        return -1;
> +    }
> +    xml[header.xml_len] = '\0';

  I would rather save xml_len to include the trailing 0 as part of the save
and simplify the + 1. For example it would be legal to have an XML description
done in UTF-16, where adding a single trailing 0 byte would not be sufficient,
better consider the lenght to be the full data, NUL character included.
  Oh and wrapping the read() here too, as suggested in previous patch.

> +    vm = qemudFindVMByUUID(driver, def->uuid);
> +    if (!vm) vm = qemudFindVMByName(driver, def->name);
> +    if (vm && qemudIsActiveVM(vm)) {
> +        qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED,
> +                         "domain to restore is already active");

  Would be great to have the name printed here in the error message, allowing
to take corrective action or at least some easy analysis

  thanks,

Daniel

-- 
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard      | virtualization library  http://libvirt.org/
veillard at redhat.com  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine  http://rpmfind.net/