[libvirt] [PATCHv2 27/27] build: add syntax check for proper flags use

[Eric Blake]

On 07/13/2011 07:35 AM, Matthias Bolte wrote:
>> @@ -516,13 +516,13 @@ virFDStreamOpenFileInternal(virStreamPtr st,
>>     int errfd = -1;
>>     pid_t pid = 0;
>>
>> -    VIR_DEBUG("st=%p path=%s flags=%x offset=%llu length=%llu mode=%o delete=%d",
>> -              st, path, flags, offset, length, mode, delete);
>> +    VIR_DEBUG("st=%p path=%s oflags=%x offset=%llu length=%llu mode=%o delete=%d",
>> +              st, path, oflags, offset, length, mode, delete);
> 
> In 02/27 you added a syntax-check rule to enforce flags=%x. Does this
> automatically cover oflags too, or does this need a change in the
> rule?

Hmm, as committed in 2/27, it checked '\<flags=%...'.  If we remove the
\< then it would also cover oflags, and that's probably a good change to
make.

I'll do it, and see what fallout it causes.

> 
>> @@ -564,7 +564,7 @@ virFDStreamOpenFileInternal(virStreamPtr st,
>>         cmd = virCommandNewArgList(LIBEXECDIR "/libvirt_iohelper",
>>                                    path,
>>                                    NULL);
>> -        virCommandAddArgFormat(cmd, "%d", flags);
>> +        virCommandAddArgFormat(cmd, "%d", oflags);
> 
> In 02/27 you changed the printing of flags from %d to %x in debug
> output. Maybe we should do that here too for consistence and adapt
> libvirt_iohelper? Or is there any possibility for a version mismatch
> between libvirt and libvirt_iohelper and we cannot change this anymore
> without breaking backwards compatibility?

I thought about this (independently, because I'm working on an O_DIRECT
patch for iohelper), and my conclusion was that normally libvirtd and
libvirt_iohelper are installed at the same time.  However, there is
indeed a window where:

Older libvirtd is running, and you install the newer executables.
Then the older libvirtd calls out to libvirt_iohelper, and executes the
new one.

That is, if you install a new libvirtd package, but don't restart the
libvirtd daemon, then the new libvirt_iohelper must understand the
syntax that will be handed it by the older still-running libvirtd.

Therefore, my conclusion is that we can't change any existing command
lines, but can only add new ones.  Adding new could include using "0x"
or "0" prefixes (the old code output decimal, so it is distinguishable,
and the new iohelper command line would then be that much easier to
understand if you browse /proc/nnn/ to learn the command line that
iohelper is using).

> 
>>         virCommandAddArgFormat(cmd, "%d", mode);
> 
> Same comment applies here about mode and switching from %d to %o.

I'll look into that, as a separate patch.

> 
> ACK.

I already have to post a v2, so we'll get another round of review on
this commit as well as the couple of fallout suggestions raised here.

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20110713/5229e4d4/attachment-0001.sig>