On 05/05/2011 11:26 AM, Daniel P. Berrange wrote:
> Given a PID, the QEMU driver reads /proc/$PID/cmdline and
> /proc/$PID/environ to get the configuration. This is fed
> into the ARGV->XML convertor to build an XML configuration
s/convertor/converter/
> +static int qemuParseProcFileStrings(int pid,
> + const char *name,
> + const char ***list)
> +{
> + char *path = NULL;
> + int ret = -1;
> + char *data = NULL;
> + ssize_t len;
> + char *tmp;
> + size_t nstr = 0;
> + const char **str = NULL;
> + int i;
> +
> + if (virAsprintf(&path, "/proc/%d/%s", pid, name) < 0) {
> + virReportOOMError();
> + goto cleanup;
> + }
> +
> + if ((len = virFileReadAll(path, 1024*128, &data)) < 0)
> + goto cleanup;
Can we ever short-change ourselves by not reading the entire file? Qemu
can have some pretty hefty command-line lengths. Then again, typing in
128k manually seems obtuse, especially since the most common user of
this feature will be developers.
> +
> + tmp = data;
> + while (tmp < (data + len)) {
> + if (VIR_EXPAND_N(str, nstr, 1) < 0) {
VIR_RESIZE_N might be nicer here, since there could be a lot of
arguments to the point of noticing the quadratic nature of VIR_EXPAND_N.
> + virReportOOMError();
> + goto cleanup;
> + }
> +
> + if (!(str[nstr-1] = strdup(tmp))) {
> + virReportOOMError();
> + goto cleanup;
> + }
> + /* Skip arg */
> + tmp += strlen(tmp);
> + /* Skip \0 separator */
> + tmp++;
> + }
> +
> + if (VIR_EXPAND_N(str, nstr, 1) < 0) {
> + virReportOOMError();
> + goto cleanup;
> + }
> +
> + str[nstr-1] = NULL;
This line is redundant - VIR_EXPAND_N guarantees that the element starts
life as NULL.
> + if (virAsprintf(&exepath, "/proc/%d/exe", pid) < 0) {
> + virReportOOMError();
> + goto cleanup;
> + }
> +
> + if (virFileResolveLink(exepath, &emulator) < 0) {
> + virReportSystemError(errno,
> + _("Unable to resolve %s"), exepath);
> + goto cleanup;
> + }
What happens if /proc/nnn/exe points to an older qemu, and qemu has been
upgraded in the meantime? In that case, the symlink resolution will
generally fail (because the kernel changes that symlink to have "
(deleted)" appended to it), but you can still exec() /proc/nnn/exe (that
is, as long as at least one process still has the old inode open, then
you can spawn other processes from that same inode, and thus properly
query the capabilities of the old qemu, rather than flat out giving up).
But in general, I can agree that it's probably safer to require that we
resolve the name than it is to just reuse /proc/%d/exe as the name of
our binary, even if that means we are not robust to qemu upgrades.
After all, this API will mark the new domain tainted.
> +#if 0
> + /*
> + * Normally PCI addresses are assigned in the virDomainCreate
> + * or virDomainDefine methods. We might still need to assign
> + * some here to cope with the question of upgrades. Regardless
> + * we also need to populate the PCi address set cache for later
s/PCi/PCI/
> +
> +cleanup:
> + /* We jump here if we failed to start the VM for any reason, or
> + * if we failed to initialize the now running VM. kill it off and
> + * pretend we never started it */
Fix the comment - we don't try to kill qemu if attach failed.
ACK with nits fixed.
--
Eric Blake eblake@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
--
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