Re: [libvirt] [PATCH v2 3/3] qemu: Automatically create NVRAM store

comments below On 08/15/14 15:43, Michal Privoznik wrote: I can see that you dropped "--with-qemu-nvram-file". This makes sense, given that we'll have a map of defaults. (1) I think you could drop this hunk. It doesn't hurt of course and helps cleaning up the configure.ac file, but not touching it at all in this series would keep the noise down. No changes relative to v1 thus far AFAICS, good... makes sense and seems consistent with other str_array_entries (then, quoting out of order:) (2) Hmmm, looking at other lists in this file, like "cgroup_controllers" and "cgroup_device_acl", I think you shouldn't indent { "nvram" itself. The four space indentation looks okay for the entries (the one entry) though. okay Now I'll have to rearrange your patch again, to set the scene: Okay, this representation (for a list of pairs) seems to match existent practice: _qemuDomainCmdlineDef. Good. I'm okay with these being here. (3) What happens when the first strdup succeeds, and the second fails? Under the "error:" label, virObjectUnref() is called, which (apparently) leads to virQEMUDriverConfigDispose(). However, virQEMUDriverConfigDispose() depends on cfg->nloader, and if the second strdup fails, nloader will be zero. Which means that cfg->loader[0] will be leaked. I think you should set "cfg->nloader" to 1 between the second alloc, and the first strdup. Then, - if the first alloc fails: - the loop in the destructor is not entered. (Correctly.) - if the second alloc fails: - the loop in the destructor is not entered. This is again correct because cfg->loader[0] will be NULL, and we won't leak anything. - if the first strdup fails: - the loop in the destructor is entered. Both [0] array elements will be NULL, but VIR_FREE can handle that - if the 2nd strdup fails: - the loop in the destructor is entered. loader[0] is released, and VIR_FREE handles the fact that nvram[0] is NULL. Q1: This assumes that "str" is non-NULL... Is that guaranteed? (--> yes, see later) virStringSplit() can return: - NULL, if out of memory, - an empty vector (ie. token[0] == NULL) if the input string is empty (4) Therefore this error message seems incorrect, because (token == NULL) means out-of-memory. I think virStringSplit() will have internally reported that though (via the allocator functions). Makes sense. (Note for self: both elements may be empty strings now, see eg " : ".) (5) The comment is inaccurate; exactly two tokens are expected. > + if (!token[0] || !token[1] || token[2] || > + STREQ(token[0], "") || STREQ(token[1], "")) { > + goto cleanup; > + } > + > + if (VIR_STRDUP(*loader, token[0]) < 0 || > + VIR_STRDUP(*nvram, token[1]) < 0) > + goto cleanup; Q2: What happens if the first stdup succeeds but the second fails? (--> will work correctly, thanks to the caller, see later) okay (this leaves the lists in consistent state even if we fail lower down) okay ... what happens if the list is empty? As in, nvram = []. This is exactly right, see what I proposed for virQEMUDriverConfigNew()! I guess I can't tell if "pp->str" is guaranteed to be non-NULL. It probably is, at worst it should be an empty string. Answers Q1. okay, this answers Q2 in virQEMUDriverConfigNVRAMParse(). This pattern happens to follow what I proposed for virQEMUDriverConfigNew(). If the first strdup succeeds in virQEMUDriverConfigNVRAMParse(), and the 2nd fails, then nvram[i] will remain NULL. But, cfg->nloader will be correctly set already, so the disposal function will do the right thing. Great. (6) Haha, no, not entirely. VIR_REALLOC_N() does *not* work like VIR_ALLOC_N(): it does *not* zero-fill the fresh area extension. Now consider the following: - you specify a list of 10 elements (ie. ten pairs) - the default size (from the default constructor) is 1 pair - in the first while loop, we kill said 1st pair (the loader[0] and nvram[0] will be zeroed by VIR_FREE) - then the reallocs succeed, appending 9 pairs of garbage - then virQEMUDriverConfigNVRAMParse() fails for whatever reason (before, between, or after the two strdups, doesn't matter), - and the disposal function will try to free a number of garbage pointers. You need to call VIR_EXPAND_N() or VIR_RESIZE_N() instead (they zero new elements). virResizeN() is based on virExpandN(), and virExpandN() is based on virReallocN(). Since we don't expand in a loop, one by one, I recommend VIR_EXPAND_N(). (7) Shouldn't we complain here if nvram is present (p != NULL), but it's not a list (p->type != VIR_CONF_LIST)? (8) I argue that you do not need to wrap safewrite() in a loop. First of all, dstFD is a regular file (you just created it with O_EXCL). Then, safewrite() itself contains a loop. And, in *that* loop, the (r == 0) condition will never fire. write() can never return 0 on a regular file *unless* you request it to write 0 bytes, but in that case, the write() isn't even reached in safewrite(). Hence, write() in safewrite() will either return: - minus one, - a positive value. Consequently, safewrite() will return with an error, or write the full buffer. (9) In addition, if we're failing and you have created the file (independently of whether you've created the pathname!), we should unlink the file. Otherwise, suppose the user just tries to start the VM *again*, after seeing the error message. This time a truncated (incomplete) varstore file might exist, from the last (failed) attempt. Such a truncated varstore should not be passed to qemu, hence it's best to delete it right when we notice the error creating it. Thanks, Laszlo