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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

> +    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@xxxxxxxxxx  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine  http://rpmfind.net/

--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]