| Message ID | 1441098383-22585-6-git-send-email-yanghy@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Sep 01, 2015 at 05:06:18PM +0800, Yang Hongyang wrote: > This will be used by the next patch in this series. > > Signed-off-by: Yang Hongyang <yanghy@cn.fujitsu.com> > Reviewed-by: Thomas Huth <thuth@redhat.com> > --- > include/net/queue.h | 19 +++++++++++++++++++ > net/queue.c | 19 ------------------- > 2 files changed, 19 insertions(+), 19 deletions(-) > > diff --git a/include/net/queue.h b/include/net/queue.h > index fc02b33..1d65e47 100644 > --- a/include/net/queue.h > +++ b/include/net/queue.h > @@ -31,6 +31,25 @@ typedef struct NetQueue NetQueue; > > typedef void (NetPacketSent) (NetClientState *sender, ssize_t ret); > > +struct NetPacket { > + QTAILQ_ENTRY(NetPacket) entry; > + NetClientState *sender; > + unsigned flags; > + int size; > + NetPacketSent *sent_cb; > + uint8_t data[0]; > +}; > + > +struct NetQueue { > + void *opaque; > + uint32_t nq_maxlen; > + uint32_t nq_count; > + > + QTAILQ_HEAD(packets, NetPacket) packets; > + > + unsigned delivering:1; > +}; > + Why is it necessary to expose both structs? Normally functions would be added to maintain the abstraction. Instead, you have chosen to access the fields directly - this is probably a bad idea.
On 09/01/2015 10:43 PM, Stefan Hajnoczi wrote: > On Tue, Sep 01, 2015 at 05:06:18PM +0800, Yang Hongyang wrote: >> This will be used by the next patch in this series. >> >> Signed-off-by: Yang Hongyang <yanghy@cn.fujitsu.com> >> Reviewed-by: Thomas Huth <thuth@redhat.com> >> --- >> include/net/queue.h | 19 +++++++++++++++++++ >> net/queue.c | 19 ------------------- >> 2 files changed, 19 insertions(+), 19 deletions(-) >> >> diff --git a/include/net/queue.h b/include/net/queue.h >> index fc02b33..1d65e47 100644 >> --- a/include/net/queue.h >> +++ b/include/net/queue.h >> @@ -31,6 +31,25 @@ typedef struct NetQueue NetQueue; >> >> typedef void (NetPacketSent) (NetClientState *sender, ssize_t ret); >> >> +struct NetPacket { >> + QTAILQ_ENTRY(NetPacket) entry; >> + NetClientState *sender; >> + unsigned flags; >> + int size; >> + NetPacketSent *sent_cb; >> + uint8_t data[0]; >> +}; >> + >> +struct NetQueue { >> + void *opaque; >> + uint32_t nq_maxlen; >> + uint32_t nq_count; >> + >> + QTAILQ_HEAD(packets, NetPacket) packets; >> + >> + unsigned delivering:1; >> +}; >> + > > Why is it necessary to expose both structs? In next patch, we add a API to pass the packet to next filter: ssize_t qemu_netfilter_pass_to_next(NetFilterState *nf, NetPacket *packet); which will access the internals of NetPacket. and in buffer filter, we need the NetQueue to Queue packets, and also to access queue->packets(pass this packets to next filter). > > Normally functions would be added to maintain the abstraction. Instead, > you have chosen to access the fields directly - this is probably a bad > idea. > . >
On Wed, Sep 02, 2015 at 09:49:18AM +0800, Yang Hongyang wrote: > On 09/01/2015 10:43 PM, Stefan Hajnoczi wrote: > >On Tue, Sep 01, 2015 at 05:06:18PM +0800, Yang Hongyang wrote: > >>This will be used by the next patch in this series. > >> > >>Signed-off-by: Yang Hongyang <yanghy@cn.fujitsu.com> > >>Reviewed-by: Thomas Huth <thuth@redhat.com> > >>--- > >> include/net/queue.h | 19 +++++++++++++++++++ > >> net/queue.c | 19 ------------------- > >> 2 files changed, 19 insertions(+), 19 deletions(-) > >> > >>diff --git a/include/net/queue.h b/include/net/queue.h > >>index fc02b33..1d65e47 100644 > >>--- a/include/net/queue.h > >>+++ b/include/net/queue.h > >>@@ -31,6 +31,25 @@ typedef struct NetQueue NetQueue; > >> > >> typedef void (NetPacketSent) (NetClientState *sender, ssize_t ret); > >> > >>+struct NetPacket { > >>+ QTAILQ_ENTRY(NetPacket) entry; > >>+ NetClientState *sender; > >>+ unsigned flags; > >>+ int size; > >>+ NetPacketSent *sent_cb; > >>+ uint8_t data[0]; > >>+}; > >>+ > >>+struct NetQueue { > >>+ void *opaque; > >>+ uint32_t nq_maxlen; > >>+ uint32_t nq_count; > >>+ > >>+ QTAILQ_HEAD(packets, NetPacket) packets; > >>+ > >>+ unsigned delivering:1; > >>+}; > >>+ > > > >Why is it necessary to expose both structs? > > In next patch, we add a API to pass the packet to next filter: > ssize_t qemu_netfilter_pass_to_next(NetFilterState *nf, NetPacket *packet); > which will access the internals of NetPacket. > > and in buffer filter, we need the NetQueue to Queue packets, and also to > access queue->packets(pass this packets to next filter). Can you do these things by introducing net queue APIs instead of exposing the struct? It's a C anti-pattern to expose structs. It makes code complex because now the net queue code no longer contains all the assumptions about NetQueue/NetPacket fields. People modifying the code now also need to go into the net filter code to figure out how this struct works. Exposing the fields also leads to code duplication since every user will reimplement similar stuff if they access the fields directly instead of using a function interface. Stefan
On 09/02/2015 09:02 PM, Stefan Hajnoczi wrote: > On Wed, Sep 02, 2015 at 09:49:18AM +0800, Yang Hongyang wrote: >> On 09/01/2015 10:43 PM, Stefan Hajnoczi wrote: >>> On Tue, Sep 01, 2015 at 05:06:18PM +0800, Yang Hongyang wrote: >>>> This will be used by the next patch in this series. >>>> >>>> Signed-off-by: Yang Hongyang <yanghy@cn.fujitsu.com> >>>> Reviewed-by: Thomas Huth <thuth@redhat.com> >>>> --- >>>> include/net/queue.h | 19 +++++++++++++++++++ >>>> net/queue.c | 19 ------------------- >>>> 2 files changed, 19 insertions(+), 19 deletions(-) >>>> >>>> diff --git a/include/net/queue.h b/include/net/queue.h >>>> index fc02b33..1d65e47 100644 >>>> --- a/include/net/queue.h >>>> +++ b/include/net/queue.h >>>> @@ -31,6 +31,25 @@ typedef struct NetQueue NetQueue; >>>> >>>> typedef void (NetPacketSent) (NetClientState *sender, ssize_t ret); >>>> >>>> +struct NetPacket { >>>> + QTAILQ_ENTRY(NetPacket) entry; >>>> + NetClientState *sender; >>>> + unsigned flags; >>>> + int size; >>>> + NetPacketSent *sent_cb; >>>> + uint8_t data[0]; >>>> +}; >>>> + >>>> +struct NetQueue { >>>> + void *opaque; >>>> + uint32_t nq_maxlen; >>>> + uint32_t nq_count; >>>> + >>>> + QTAILQ_HEAD(packets, NetPacket) packets; >>>> + >>>> + unsigned delivering:1; >>>> +}; >>>> + >>> >>> Why is it necessary to expose both structs? >> >> In next patch, we add a API to pass the packet to next filter: >> ssize_t qemu_netfilter_pass_to_next(NetFilterState *nf, NetPacket *packet); >> which will access the internals of NetPacket. >> >> and in buffer filter, we need the NetQueue to Queue packets, and also to >> access queue->packets(pass this packets to next filter). > > Can you do these things by introducing net queue APIs instead of > exposing the struct? > > It's a C anti-pattern to expose structs. It makes code complex because > now the net queue code no longer contains all the assumptions about > NetQueue/NetPacket fields. People modifying the code now also need to > go into the net filter code to figure out how this struct works. > > Exposing the fields also leads to code duplication since every user will > reimplement similar stuff if they access the fields directly instead of > using a function interface. I can't think of a interface that no need to access the NetPacket internals to archive the needs. Except I do not reuse the NetPacket and NetQueue, implement a Queue myself. But that will be more complex. So in order to not expose those structs, I shall add lots of public get functions to use these internals, like: Qemu_NetQueue_get_xxx Qemu_NetPacket_get_xxx I'd be very happy if you could give me a better suggestion on this. Thanks! > > Stefan > . >
On Thu, Sep 03, 2015 at 12:18:18AM +0800, Yang Hongyang wrote: > > > On 09/02/2015 09:02 PM, Stefan Hajnoczi wrote: > >On Wed, Sep 02, 2015 at 09:49:18AM +0800, Yang Hongyang wrote: > >>On 09/01/2015 10:43 PM, Stefan Hajnoczi wrote: > >>>On Tue, Sep 01, 2015 at 05:06:18PM +0800, Yang Hongyang wrote: > >>>>This will be used by the next patch in this series. > >>>> > >>>>Signed-off-by: Yang Hongyang <yanghy@cn.fujitsu.com> > >>>>Reviewed-by: Thomas Huth <thuth@redhat.com> > >>>>--- > >>>> include/net/queue.h | 19 +++++++++++++++++++ > >>>> net/queue.c | 19 ------------------- > >>>> 2 files changed, 19 insertions(+), 19 deletions(-) > >>>> > >>>>diff --git a/include/net/queue.h b/include/net/queue.h > >>>>index fc02b33..1d65e47 100644 > >>>>--- a/include/net/queue.h > >>>>+++ b/include/net/queue.h > >>>>@@ -31,6 +31,25 @@ typedef struct NetQueue NetQueue; > >>>> > >>>> typedef void (NetPacketSent) (NetClientState *sender, ssize_t ret); > >>>> > >>>>+struct NetPacket { > >>>>+ QTAILQ_ENTRY(NetPacket) entry; > >>>>+ NetClientState *sender; > >>>>+ unsigned flags; > >>>>+ int size; > >>>>+ NetPacketSent *sent_cb; > >>>>+ uint8_t data[0]; > >>>>+}; > >>>>+ > >>>>+struct NetQueue { > >>>>+ void *opaque; > >>>>+ uint32_t nq_maxlen; > >>>>+ uint32_t nq_count; > >>>>+ > >>>>+ QTAILQ_HEAD(packets, NetPacket) packets; > >>>>+ > >>>>+ unsigned delivering:1; > >>>>+}; > >>>>+ > >>> > >>>Why is it necessary to expose both structs? > >> > >>In next patch, we add a API to pass the packet to next filter: > >>ssize_t qemu_netfilter_pass_to_next(NetFilterState *nf, NetPacket *packet); > >>which will access the internals of NetPacket. > >> > >>and in buffer filter, we need the NetQueue to Queue packets, and also to > >>access queue->packets(pass this packets to next filter). > > > >Can you do these things by introducing net queue APIs instead of > >exposing the struct? > > > >It's a C anti-pattern to expose structs. It makes code complex because > >now the net queue code no longer contains all the assumptions about > >NetQueue/NetPacket fields. People modifying the code now also need to > >go into the net filter code to figure out how this struct works. > > > >Exposing the fields also leads to code duplication since every user will > >reimplement similar stuff if they access the fields directly instead of > >using a function interface. > > I can't think of a interface that no need to access the NetPacket internals > to archive the needs. Except I do not reuse the NetPacket and NetQueue, > implement a Queue myself. But that will be more complex. > So in order to not expose those structs, I shall add lots of public get > functions to use these internals, like: > Qemu_NetQueue_get_xxx > Qemu_NetPacket_get_xxx > > I'd be very happy if you could give me a better suggestion on this. net/queue.c has logic to send/queue/flush packets but a qemu_deliver_packet() call is hardcoded. Maybe you can extend qemu_new_net_queue() like this: /* Returns: * >0 - success * 0 - queue packet for future redelivery * <0 - failure (discard packet) */ typedef ssize_t NetQueueDeliverFunc(NetClientState *sender, unsigned flags, const struct iovec *iov, int iovcnt, void *opaque); NetQueue *qemu_new_net_queue(NetQueueDeliverFunc deliver, void *opaque); Now net/net.c:qemu_net_client_setup() needs to call: nc->incoming_queue = qemu_new_net_queue(qemu_deliver_packet_iov, nc); And the filter code can use qemu_net_queue_send_iov() and qemu_net_queue_flush(). The filter just needs to provide its own NetQueueDeliveryFunc. I haven't checked the details (e.g. non-iov delivery, etc) but the idea is to use the net/queue.c API instead of duplicating similar logic in the filter code.
Hi Stefan, On 09/04/2015 06:32 PM, Stefan Hajnoczi wrote: [...] > > net/queue.c has logic to send/queue/flush packets but a > qemu_deliver_packet() call is hardcoded. > > Maybe you can extend qemu_new_net_queue() like this: > > /* Returns: > * >0 - success > * 0 - queue packet for future redelivery > * <0 - failure (discard packet) > */ > typedef ssize_t NetQueueDeliverFunc(NetClientState *sender, > unsigned flags, > const struct iovec *iov, > int iovcnt, > void *opaque); > > NetQueue *qemu_new_net_queue(NetQueueDeliverFunc deliver, > void *opaque); > > Now net/net.c:qemu_net_client_setup() needs to call: > > nc->incoming_queue = qemu_new_net_queue(qemu_deliver_packet_iov, nc); > > And the filter code can use qemu_net_queue_send_iov() and > qemu_net_queue_flush(). The filter just needs to provide its own > NetQueueDeliveryFunc. > > I haven't checked the details (e.g. non-iov delivery, etc) but the idea > is to use the net/queue.c API instead of duplicating similar logic in > the filter code. Thanks very much for the suggestion, I've already implemented it and tested, the code looks cleaner now. The last issue is the QOM thing, do Markus and Andreas have more input about that? > . >
Yang Hongyang <yanghy@cn.fujitsu.com> writes: > Hi Stefan, > > On 09/04/2015 06:32 PM, Stefan Hajnoczi wrote: > [...] >> >> net/queue.c has logic to send/queue/flush packets but a >> qemu_deliver_packet() call is hardcoded. >> >> Maybe you can extend qemu_new_net_queue() like this: >> >> /* Returns: >> * >0 - success >> * 0 - queue packet for future redelivery >> * <0 - failure (discard packet) >> */ >> typedef ssize_t NetQueueDeliverFunc(NetClientState *sender, >> unsigned flags, >> const struct iovec *iov, >> int iovcnt, >> void *opaque); >> >> NetQueue *qemu_new_net_queue(NetQueueDeliverFunc deliver, >> void *opaque); >> >> Now net/net.c:qemu_net_client_setup() needs to call: >> >> nc->incoming_queue = qemu_new_net_queue(qemu_deliver_packet_iov, nc); >> >> And the filter code can use qemu_net_queue_send_iov() and >> qemu_net_queue_flush(). The filter just needs to provide its own >> NetQueueDeliveryFunc. >> >> I haven't checked the details (e.g. non-iov delivery, etc) but the idea >> is to use the net/queue.c API instead of duplicating similar logic in >> the filter code. > > Thanks very much for the suggestion, I've already implemented it and tested, > the code looks cleaner now. > > The last issue is the QOM thing, do Markus and Andreas have more input > about that? This series is in my review queue. I'm struggling with clearing my queue, and apologize for the delay.
On Mon, Sep 07, 2015 at 03:37:20PM +0800, Yang Hongyang wrote: > Hi Stefan, > > On 09/04/2015 06:32 PM, Stefan Hajnoczi wrote: > [...] > > > >net/queue.c has logic to send/queue/flush packets but a > >qemu_deliver_packet() call is hardcoded. > > > >Maybe you can extend qemu_new_net_queue() like this: > > > >/* Returns: > > * >0 - success > > * 0 - queue packet for future redelivery > > * <0 - failure (discard packet) > > */ > >typedef ssize_t NetQueueDeliverFunc(NetClientState *sender, > > unsigned flags, > > const struct iovec *iov, > > int iovcnt, > > void *opaque); > > > >NetQueue *qemu_new_net_queue(NetQueueDeliverFunc deliver, > > void *opaque); > > > >Now net/net.c:qemu_net_client_setup() needs to call: > > > > nc->incoming_queue = qemu_new_net_queue(qemu_deliver_packet_iov, nc); > > > >And the filter code can use qemu_net_queue_send_iov() and > >qemu_net_queue_flush(). The filter just needs to provide its own > >NetQueueDeliveryFunc. > > > >I haven't checked the details (e.g. non-iov delivery, etc) but the idea > >is to use the net/queue.c API instead of duplicating similar logic in > >the filter code. > > Thanks very much for the suggestion, I've already implemented it and tested, > the code looks cleaner now. > > The last issue is the QOM thing, do Markus and Andreas have more input > about that? If you would like to see examples of QOM usage, take a look at iothread.c and/or backends/hostmem.c. The key things are: 1. They use include/qom/object.h to register a type based on TYPE_OBJECT and their properties are registered using object_property_add_*(). 2. They implement the TYPE_USER_CREATABLE interface so the -object command-line option can be used to instantiate them. See object_interfaces.h. As a result, a lot of code becomes unnecessary and iothread.c, in particular, is quite short. Stefan
On 09/07/2015 05:06 PM, Markus Armbruster wrote: > Yang Hongyang <yanghy@cn.fujitsu.com> writes: > >> Hi Stefan, >> >> On 09/04/2015 06:32 PM, Stefan Hajnoczi wrote: >> [...] >>> >>> net/queue.c has logic to send/queue/flush packets but a >>> qemu_deliver_packet() call is hardcoded. >>> >>> Maybe you can extend qemu_new_net_queue() like this: >>> >>> /* Returns: >>> * >0 - success >>> * 0 - queue packet for future redelivery >>> * <0 - failure (discard packet) >>> */ >>> typedef ssize_t NetQueueDeliverFunc(NetClientState *sender, >>> unsigned flags, >>> const struct iovec *iov, >>> int iovcnt, >>> void *opaque); >>> >>> NetQueue *qemu_new_net_queue(NetQueueDeliverFunc deliver, >>> void *opaque); >>> >>> Now net/net.c:qemu_net_client_setup() needs to call: >>> >>> nc->incoming_queue = qemu_new_net_queue(qemu_deliver_packet_iov, nc); >>> >>> And the filter code can use qemu_net_queue_send_iov() and >>> qemu_net_queue_flush(). The filter just needs to provide its own >>> NetQueueDeliveryFunc. >>> >>> I haven't checked the details (e.g. non-iov delivery, etc) but the idea >>> is to use the net/queue.c API instead of duplicating similar logic in >>> the filter code. >> >> Thanks very much for the suggestion, I've already implemented it and tested, >> the code looks cleaner now. >> >> The last issue is the QOM thing, do Markus and Andreas have more input >> about that? > > This series is in my review queue. I'm struggling with clearing my > queue, and apologize for the delay. Thanks, I'm going to rebase this series on QOM in v10, the QAPI part will be the same as in this series unless you're about to review this series, so if you are still struggling on the long queue, you might want to review the next version :) > . >
On 09/07/2015 05:11 PM, Stefan Hajnoczi wrote: > On Mon, Sep 07, 2015 at 03:37:20PM +0800, Yang Hongyang wrote: >> Hi Stefan, >> >> On 09/04/2015 06:32 PM, Stefan Hajnoczi wrote: >> [...] >>> >>> net/queue.c has logic to send/queue/flush packets but a >>> qemu_deliver_packet() call is hardcoded. >>> >>> Maybe you can extend qemu_new_net_queue() like this: >>> >>> /* Returns: >>> * >0 - success >>> * 0 - queue packet for future redelivery >>> * <0 - failure (discard packet) >>> */ >>> typedef ssize_t NetQueueDeliverFunc(NetClientState *sender, >>> unsigned flags, >>> const struct iovec *iov, >>> int iovcnt, >>> void *opaque); >>> >>> NetQueue *qemu_new_net_queue(NetQueueDeliverFunc deliver, >>> void *opaque); >>> >>> Now net/net.c:qemu_net_client_setup() needs to call: >>> >>> nc->incoming_queue = qemu_new_net_queue(qemu_deliver_packet_iov, nc); >>> >>> And the filter code can use qemu_net_queue_send_iov() and >>> qemu_net_queue_flush(). The filter just needs to provide its own >>> NetQueueDeliveryFunc. >>> >>> I haven't checked the details (e.g. non-iov delivery, etc) but the idea >>> is to use the net/queue.c API instead of duplicating similar logic in >>> the filter code. >> >> Thanks very much for the suggestion, I've already implemented it and tested, >> the code looks cleaner now. >> >> The last issue is the QOM thing, do Markus and Andreas have more input >> about that? > > If you would like to see examples of QOM usage, take a look at > iothread.c and/or backends/hostmem.c. > > The key things are: > > 1. They use include/qom/object.h to register a type based on > TYPE_OBJECT and their properties are registered using > object_property_add_*(). > > 2. They implement the TYPE_USER_CREATABLE interface so the -object > command-line option can be used to instantiate them. See > object_interfaces.h. > > As a result, a lot of code becomes unnecessary and iothread.c, in > particular, is quite short. Thanks a lot, this is what I need, will look into this and rebase this series on top of QOM. > > Stefan > . >
On 09/07/2015 05:11 PM, Stefan Hajnoczi wrote: [...] >> Thanks very much for the suggestion, I've already implemented it and tested, >> the code looks cleaner now. >> >> The last issue is the QOM thing, do Markus and Andreas have more input >> about that? > > If you would like to see examples of QOM usage, take a look at > iothread.c and/or backends/hostmem.c. > > The key things are: > > 1. They use include/qom/object.h to register a type based on > TYPE_OBJECT and their properties are registered using > object_property_add_*(). > > 2. They implement the TYPE_USER_CREATABLE interface so the -object > command-line option can be used to instantiate them. See > object_interfaces.h. > > As a result, a lot of code becomes unnecessary and iothread.c, in > particular, is quite short. After looking into this, I have some questions on the implement, could you please help me on this because I don't know much about the object mechanism: The netfilter need to be initialized after the net_init_clients, because we need to attach the filter to the net client. But currently, net client is not using QOM, and seems that it is initialized after objects been created. So here comes the problem: how can I initialize a certain object later, is it possiable? > > Stefan > . >
On Mon, Sep 07, 2015 at 06:53:49PM +0800, Yang Hongyang wrote: > On 09/07/2015 05:11 PM, Stefan Hajnoczi wrote: > [...] > >>Thanks very much for the suggestion, I've already implemented it and tested, > >>the code looks cleaner now. > >> > >>The last issue is the QOM thing, do Markus and Andreas have more input > >>about that? > > > >If you would like to see examples of QOM usage, take a look at > >iothread.c and/or backends/hostmem.c. > > > >The key things are: > > > >1. They use include/qom/object.h to register a type based on > > TYPE_OBJECT and their properties are registered using > > object_property_add_*(). > > > >2. They implement the TYPE_USER_CREATABLE interface so the -object > > command-line option can be used to instantiate them. See > > object_interfaces.h. > > > >As a result, a lot of code becomes unnecessary and iothread.c, in > >particular, is quite short. > > After looking into this, I have some questions on the implement, could you > please help me on this because I don't know much about the object mechanism: > > The netfilter need to be initialized after the net_init_clients, because we > need to attach the filter to the net client. But currently, net client is not > using QOM, and seems that it is initialized after objects been created. So here > comes the problem: how can I initialize a certain object later, is it possiable? It is currently a bit hacky - most objects are initialized very early, but we have a similar problem with rng-egd which must be created /after/ chardevs. To deal with this in vl.c main() we have two helper methods object_create_initial and object_create_delayed. The delayed method though still happens before the net clients are created. We could probably just move the objec_create_delayed method invokation to later on, after net clients are created, but if that doesn't work just add an extra helper object_create_very_delayed :-) Regards, Daniel
On 09/07/2015 07:00 PM, Daniel P. Berrange wrote: > On Mon, Sep 07, 2015 at 06:53:49PM +0800, Yang Hongyang wrote: >> On 09/07/2015 05:11 PM, Stefan Hajnoczi wrote: >> [...] >>>> Thanks very much for the suggestion, I've already implemented it and tested, >>>> the code looks cleaner now. >>>> >>>> The last issue is the QOM thing, do Markus and Andreas have more input >>>> about that? >>> >>> If you would like to see examples of QOM usage, take a look at >>> iothread.c and/or backends/hostmem.c. >>> >>> The key things are: >>> >>> 1. They use include/qom/object.h to register a type based on >>> TYPE_OBJECT and their properties are registered using >>> object_property_add_*(). >>> >>> 2. They implement the TYPE_USER_CREATABLE interface so the -object >>> command-line option can be used to instantiate them. See >>> object_interfaces.h. >>> >>> As a result, a lot of code becomes unnecessary and iothread.c, in >>> particular, is quite short. >> >> After looking into this, I have some questions on the implement, could you >> please help me on this because I don't know much about the object mechanism: >> >> The netfilter need to be initialized after the net_init_clients, because we >> need to attach the filter to the net client. But currently, net client is not >> using QOM, and seems that it is initialized after objects been created. So here >> comes the problem: how can I initialize a certain object later, is it possiable? > > It is currently a bit hacky - most objects are initialized very early, > but we have a similar problem with rng-egd which must be created /after/ > chardevs. To deal with this in vl.c main() we have two helper methods > object_create_initial and object_create_delayed. The delayed method though > still happens before the net clients are created. We could probably just > move the objec_create_delayed method invokation to later on, after net > clients are created, but if that doesn't work just add an extra helper > object_create_very_delayed :-) If it's ok to move creation of "rng-egd" later, then "move the objec_create_delayed method invokation after net clients are created" should solve the problem. Thank you for the help! > > Regards, > Daniel >
On Mon, Sep 07, 2015 at 07:41:26PM +0800, Yang Hongyang wrote: > > > On 09/07/2015 07:00 PM, Daniel P. Berrange wrote: > >On Mon, Sep 07, 2015 at 06:53:49PM +0800, Yang Hongyang wrote: > >>On 09/07/2015 05:11 PM, Stefan Hajnoczi wrote: > >>[...] > >>>>Thanks very much for the suggestion, I've already implemented it and tested, > >>>>the code looks cleaner now. > >>>> > >>>>The last issue is the QOM thing, do Markus and Andreas have more input > >>>>about that? > >>> > >>>If you would like to see examples of QOM usage, take a look at > >>>iothread.c and/or backends/hostmem.c. > >>> > >>>The key things are: > >>> > >>>1. They use include/qom/object.h to register a type based on > >>> TYPE_OBJECT and their properties are registered using > >>> object_property_add_*(). > >>> > >>>2. They implement the TYPE_USER_CREATABLE interface so the -object > >>> command-line option can be used to instantiate them. See > >>> object_interfaces.h. > >>> > >>>As a result, a lot of code becomes unnecessary and iothread.c, in > >>>particular, is quite short. > >> > >>After looking into this, I have some questions on the implement, could you > >>please help me on this because I don't know much about the object mechanism: > >> > >>The netfilter need to be initialized after the net_init_clients, because we > >>need to attach the filter to the net client. But currently, net client is not > >>using QOM, and seems that it is initialized after objects been created. So here > >>comes the problem: how can I initialize a certain object later, is it possiable? > > > >It is currently a bit hacky - most objects are initialized very early, > >but we have a similar problem with rng-egd which must be created /after/ > >chardevs. To deal with this in vl.c main() we have two helper methods > >object_create_initial and object_create_delayed. The delayed method though > >still happens before the net clients are created. We could probably just > >move the objec_create_delayed method invokation to later on, after net > >clients are created, but if that doesn't work just add an extra helper > >object_create_very_delayed :-) > > If it's ok to move creation of "rng-egd" later, then "move the > objec_create_delayed method invokation after net clients are created" should > solve the problem. I think it should be ok, because IIRC, the only requirement from rng-egd was that it be created /after/ -device is processed, so moving it even later shouldn't hurt it Regards, Daniel
On 09/07/2015 07:43 PM, Daniel P. Berrange wrote: > On Mon, Sep 07, 2015 at 07:41:26PM +0800, Yang Hongyang wrote: >> >> >> On 09/07/2015 07:00 PM, Daniel P. Berrange wrote: >>> On Mon, Sep 07, 2015 at 06:53:49PM +0800, Yang Hongyang wrote: >>>> On 09/07/2015 05:11 PM, Stefan Hajnoczi wrote: >>>> [...] >>>>>> Thanks very much for the suggestion, I've already implemented it and tested, >>>>>> the code looks cleaner now. >>>>>> >>>>>> The last issue is the QOM thing, do Markus and Andreas have more input >>>>>> about that? >>>>> >>>>> If you would like to see examples of QOM usage, take a look at >>>>> iothread.c and/or backends/hostmem.c. >>>>> >>>>> The key things are: >>>>> >>>>> 1. They use include/qom/object.h to register a type based on >>>>> TYPE_OBJECT and their properties are registered using >>>>> object_property_add_*(). >>>>> >>>>> 2. They implement the TYPE_USER_CREATABLE interface so the -object >>>>> command-line option can be used to instantiate them. See >>>>> object_interfaces.h. >>>>> >>>>> As a result, a lot of code becomes unnecessary and iothread.c, in >>>>> particular, is quite short. >>>> >>>> After looking into this, I have some questions on the implement, could you >>>> please help me on this because I don't know much about the object mechanism: >>>> >>>> The netfilter need to be initialized after the net_init_clients, because we >>>> need to attach the filter to the net client. But currently, net client is not >>>> using QOM, and seems that it is initialized after objects been created. So here >>>> comes the problem: how can I initialize a certain object later, is it possiable? >>> >>> It is currently a bit hacky - most objects are initialized very early, >>> but we have a similar problem with rng-egd which must be created /after/ >>> chardevs. To deal with this in vl.c main() we have two helper methods >>> object_create_initial and object_create_delayed. The delayed method though >>> still happens before the net clients are created. We could probably just >>> move the objec_create_delayed method invokation to later on, after net >>> clients are created, but if that doesn't work just add an extra helper >>> object_create_very_delayed :-) >> >> If it's ok to move creation of "rng-egd" later, then "move the >> objec_create_delayed method invokation after net clients are created" should >> solve the problem. > > I think it should be ok, because IIRC, the only requirement from rng-egd > was that it be created /after/ -device is processed, so moving it even > later shouldn't hurt it Got it, thanks! > > Regards, > Daniel >
diff --git a/include/net/queue.h b/include/net/queue.h index fc02b33..1d65e47 100644 --- a/include/net/queue.h +++ b/include/net/queue.h @@ -31,6 +31,25 @@ typedef struct NetQueue NetQueue; typedef void (NetPacketSent) (NetClientState *sender, ssize_t ret); +struct NetPacket { + QTAILQ_ENTRY(NetPacket) entry; + NetClientState *sender; + unsigned flags; + int size; + NetPacketSent *sent_cb; + uint8_t data[0]; +}; + +struct NetQueue { + void *opaque; + uint32_t nq_maxlen; + uint32_t nq_count; + + QTAILQ_HEAD(packets, NetPacket) packets; + + unsigned delivering:1; +}; + #define QEMU_NET_PACKET_FLAG_NONE 0 #define QEMU_NET_PACKET_FLAG_RAW (1<<0) diff --git a/net/queue.c b/net/queue.c index ebbe2bb..1499479 100644 --- a/net/queue.c +++ b/net/queue.c @@ -39,25 +39,6 @@ * unbounded queueing. */ -struct NetPacket { - QTAILQ_ENTRY(NetPacket) entry; - NetClientState *sender; - unsigned flags; - int size; - NetPacketSent *sent_cb; - uint8_t data[0]; -}; - -struct NetQueue { - void *opaque; - uint32_t nq_maxlen; - uint32_t nq_count; - - QTAILQ_HEAD(packets, NetPacket) packets; - - unsigned delivering : 1; -}; - NetQueue *qemu_new_net_queue(void *opaque) { NetQueue *queue;