Re: [libvirt] [PATCH v3 2/2] add interface for blkio.weight_device

On 09/23/2011 12:40 AM, Hu Tao wrote: This for loop seems like something that will be done frequently enough that it might be worth factoring it into a util file for managing virTypedParameters. Something like: virTypedParameterFree(params). Also mention that this name refers to a VIR_TYPED_PARAMETER_STRING. s/qulaified/qualified/ This stat() ties the parse to the existence of the node. But that seems wrong - it should be possible to define a domain with XML that refers to a node that does not yet exist, provided that the node later exists at the time we try to start the domain. Also, breaking the parse into major/minor this early makes the result unmigratable, since we can't guarantee major/minor stability across hosts. I think you have to store the name, not the major/minor, here in the domain_conf representation of the device weight, and only convert to major/minor in the hypervisor that is actually enforcing the limits. atoi is unsafe. It cannot detect errors. You should use virStrToLong_i (or similar) instead. (Stupid Thunderbird for munging whitespace). This will conflict with my patch series for indenting <domainsnapshot>. Shouldn't be too hard to figure out, but it boils down to who gets acked first. This should use "c-ctype.h", not <ctype.h>. This returns -1 without an error, but you later return -1 after reporting OOM error, so the caller can't tell things apart. That means you need to report an error here. On the other hand, since this function is static, you could audit that all callers pass correct parameters in the first place, and just omit this validation step. What happens if the absolute path of a device contains a literal ';'? I guess we don't anticipate that happening. strchr(p,',') would be faster. strndup() would be better than VIR_ALLOC_N and memcpy. major() and minor() are non-portable; this code may need to be conditionally compiled for non-Linux platforms. See how cgroup does conditional compilation before accessing major and minor numbers. s/isdigit/c_isdigit/ No atoi. Use virStrToLong_i or similar. You forgot to update the virCheckFlags to allow the new VIR_DOMAIN_TYPED_STRING_OKAY flag. (Actually, I'm starting to think it might be better to name that flag VIR_TYPED_PARAM_STRING_OKAY, since it only makes sense with any API that uses virTypedParameter. Here, you should also fail if flags does not include the OKAY flag, as that indicates a client that isn't properly using the API. You should report OOM errors if allocating weight_device failed, rather than silently ignoring them. If someone calls GetBlkioParameters with a size of 0 but without the _OKAY flag, to learn how large to allocate the array, then you need to return 1, not 2. I don't see that in your patch (I only see where if they allocate 2, but didn't pass _OKAY, that you don't populate the second entry). s/per device/per-device/ Do not expose major:minor,weight through virsh. Rather, you should match the xml, and expose this via /path/to/device,weight (you got it right in virsh.pod). Won't work as written. nparams will be -1, not 0, if the old server didn't understand the _OKAY flag. Also, you should check last_error->code, and only try the fallback if the error is VIR_ERR_INVALID_ARG (any other error is fatal without having to try the fallback). Why not just: temp->value.s = weight_device, instead of going through a virBuffer? You need to fix the logic to pass _OKAY if weight_device was specified. Also, I think your logic is not quite right in that you pre-set flags to include _OKAY even if you are not passing weight_device, which will cause needless problems when talking to an older server if you aren't even going to be changing device weight. s/patch/path/g -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org