Re: [libvirt] [PATCH 08.5/12] qemu: Create a set of macros to handle setting block iotune values

>> >> Almost, but what I had in mind was to just keep one of the macros - be >> it SET_IOTUNE or SET_IOTUNE_FIELD or whatever the naming is - and then >> end up with a snippet like the one in the attached patch. >> >> Erik >> >> PS: Sorry for attaching a patch, but thunderbird's editor truly sucks >> (pasting from vim is a total nightmare)...big time...and I really have >> to switch to some normal mail client. >> > > If you 'temporarily' turn off line wrapping for t-bird, then cut-n-paste > isn't too bad, but attaching a patch was fine. > Yeah, I could have done that, adjusting the message I sent along with the patch (I also tried selective wrap but that certainly didn't bring satisfactory results). Instead, I rage quit and thought that attaching the patch might not just be the worst possible way at all. Anyway, I'm starting to convince myself that spending several hours configuring mutt might as well turn out as a plausible solution of all email-related future problems than sticking with thunderbird after all.

> Suffice to say what you propose is where I initially started. Then I > looked at the extended repetition of lines where the only difference was > "_max"/"_MAX" and "_max_length"/"_MAX_LENGTH" and felt it was better to > further collapse the way I did... > > In any case, see patch 10 for another instance of "parsing" 2 things at > a time (PARSE_IOTUNE) and patch 11 for adding the _max_length to that. > In fact the patch 11 shows my entrails with the commented out macros > (since removed from my branch). > > I chose not do the same in patch 4 (IOTUNE_ADD) or patch 9 > (FORMAT_IOTUNE) because I didn't want to mess with the .args or .xml > output "adjustments" (e.g. output files). > > Personally I'd rather see the 3 variables close to each other rather > than hunting through the various output trying to see which are set. Way > too similarity that can "fool" the eyes. > > I'd prefer to keep the version of the macro I have since it doesn't > affect the output/visible order. > > John > Well, I do see your point, I just have a different feeling about that. When I look at the qemuDomainSetBlockIoTune function, I see a bunch of variables defined, among which all the set_(bytes|iops) variables reside. Let's consider a massive function where you introduced a macro to get rid of all the duplicate code. What I expect from the macro is that I clearly see what parameters it takes so I can very simply identify which variables will be affected by the macro whatever it is trying to accomplish. By introducing another macro representing an additional meta layer of name processing, I fail to see what arguments will be eventually affected by the macro machinery at first glance. Your initial idea which was identical to my proposal makes it clear what variables will be affected by the macro and the only thing that needs to be handled is prefixing the enums with VIR_DOMAIN_BLOCK_IOTUNE. In my opinion, by visually grouping the related blocks of code can also lead to certain amount of cleanliness. Another thing, since in patch 4 and 9 you went the way I'm currently describing, I'm not a fan of treating essentially the same thing differently.