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

Re: [libvirt] [PATCH 1/2] build: avoid close, system



On 01/28/2011 03:39 PM, Matthias Bolte wrote:
>> +    if (!editor)
>> +        editor = "vi"; /* could be cruel & default to ed(1) here */
> 
> When VISUAL and EDITOR isn't set we fallback to vi here, but ...
> 
>> +        cmd = virCommandNewArgList("sh", "-c", NULL);
>> +        virCommandAddArgFormat(cmd, "%s %s", editor, filename);
>> +    } else {
>> +        cmd = virCommandNewArgList("editor", filename, NULL);

AARGH - stupid typo.  That should be 'editor', not '"editor"'.  (Can you
tell that I tested the patch with EDITOR='emacs -nw' in my environment,
and not with EDITOR unset or a simple shell word?)

> 
> Anyway, that's minor and doesn't affect my ACK.

It does affect me applying the patch, though.

Ooh, one other bug.  system() shares stdin with the child process
(important if you invoke 'virsh' interactively, and your editor wants to
reuse the same stdin as virsh for the duration of virsh being blocked).
 But virCommand defaults to redirecting stdin from /dev/null (which
would make such an editor a nop, because stdin would be at EOF).  v2
coming up.

-- 
Eric Blake   eblake redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


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