Skip to contentSkip to conversation

spice-char: fix wrong assert condition

PATCH In review Zhang Haoyu 2 participants 4 messages Jan 17, 2015 – Jan 19, 2015

lore
Share & tools
lore

Zhang Haoyu

author
#0 lore raw

PATCH 1/1 spice-char: fix wrong assert condition

G_IO_OUT|G_IO_HUP are passed from all of the callers of chr_add_watch hooker, the assert condition MUST be changed.

Signed-off-by Zhang Haoyu <[email protected]>
---
 spice-qemu-char.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
spice-qemu-char.c +1 -1
index 7e0d300..b81a9db 100644
@@ -169,7 +169,7 @@ static GSource *spice_chr_add_watch(CharDriverState *chr, GIOCondition cond)
     SpiceCharDriver *scd = chr->opaque;
     SpiceCharSource *src;
 
-    assert(cond == G_IO_OUT);
+    assert(cond == (G_IO_OUT|G_IO_HUP));
 
     src = (SpiceCharSource *)g_source_new(&SpiceCharSourceFuncs,
                                           sizeof(SpiceCharSource));
Signature · 1 line
1.7.12.4
#1 lore raw
Quoted 3 lines · Zhang Haoyu

G_IO_OUT|G_IO_HUP are passed from all of the callers of chr_add_watch hooker, the assert condition MUST be changed.

"All the callers do this currently" isn't a reason to change an assert. Possible reasons to change it include:
* "X is required because if we don't then things break because Y"
* "it is meaningless (a programming error) to do X, because Y"

Can you give the reasoning for changing this assert? Does it apply also to other add_watch hook functions? Would the assert be better at a higher level in the callstack?

thanks
-- PMM

#2 lore raw
Quoted 5 lines · Peter Maydell
Quoted 3 lines · Zhang Haoyu

G_IO_OUT|G_IO_HUP are passed from all of the callers of chr_add_watch hooker, the assert condition MUST be changed.

Quoted 8 lines

"All the callers do this currently" isn't a reason to change an assert. Possible reasons to change it include:
* "X is required because if we don't then things break because Y"
* "it is meaningless (a programming error) to do X, because Y"

Can you give the reasoning for changing this assert? Does it apply also to other add_watch hook functions? Would the assert be better at a higher level in the callstack?

...that said, I've just figured out what your patch is trying to fix. I think that kind of makes the point about the commit message not really being very clear about why the change is being made, though. (Should we be checking
'(cond & G_IO_OUT) != 0'? Is this a bandaid over missing functionality in the spice chardriver?)

-- PMM

author
#3 lore raw
Quoted 15 lines · Peter Maydell
Quoted 5 lines · Peter Maydell
Quoted 3 lines · Zhang Haoyu

G_IO_OUT|G_IO_HUP are passed from all of the callers of chr_add_watch hooker, the assert condition MUST be changed.

Quoted 8 lines

"All the callers do this currently" isn't a reason to change an assert. Possible reasons to change it include:
* "X is required because if we don't then things break because Y"
* "it is meaningless (a programming error) to do X, because Y"

Can you give the reasoning for changing this assert? Does it apply also to other add_watch hook functions? Would the assert be better at a higher level in the callstack?

Quoted 5 lines

...that said, I've just figured out what your patch is trying to fix. I think that kind of makes the point about the commit message not really being very clear about why the change is being made, though. (Should we be checking
'(cond & G_IO_OUT) != 0'?

I think '(cond & G_IO_OUT) != 0' is better, v2 will be posted.

Quoted 2 lines

Is this a bandaid over missing
functionality in the spice chardriver?)

I really don't know about this.

Quoted 2 lines

-- PMM