| Message ID | 1386077165-19577-4-git-send-email-benoit@irqsave.net |
|---|---|
| State | New |
| Headers | show |
On 2013年12月03日 21:26, Benoît Canet wrote: > --- > blockdev.c | 8 ++++++++ > qapi-schema.json | 32 ++++++++++++++++++++++++++++++++ > 2 files changed, 40 insertions(+) > > diff --git a/blockdev.c b/blockdev.c > index a474bb5..824e718 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -1940,6 +1940,14 @@ void qmp_drive_backup(const char *device, const char *target, > } > } > > +BlockGraphNode * qmp_query_drive_graph(const char *device, Error **errp) > +{ > + /* the implementation of this function would recurse through the > + * BlockDriverState graph to build it's result s/it's/its/ > > ## > +# @BlockGraphNode > +# > +# Information about a node of the block driver state graph > +# > +# @node-name: the name of the node in the graph > +# > +# @drv: the name of the block format used by this node as described in > +# @BlockDeviceInfo. > +# > +# @children: a list of @BlockGraphNode being the children of this node > +# > +# Since 1.8 Since 2.0 > +## > +{ 'type': 'BlockGraphNode', > + 'data': { 'node-name': 'str', 'drv': 'str', 'children': ['BlockGraphNode'] } } > + > +## > +# @query-drive-graph > +# > +# Get the block driver states graph for a given drive > +# > +# @device: the name of the device to get the graph from > +# > +# Returns: the root @BlockGraphNode > +# > +# Since 1.8 Same as above. Fam
On 12/03/2013 06:26 AM, Benoît Canet wrote: In addition to Fam's review, s/skeletton/skeleton/ in subject > --- > blockdev.c | 8 ++++++++ > qapi-schema.json | 32 ++++++++++++++++++++++++++++++++ > 2 files changed, 40 insertions(+) > > diff --git a/blockdev.c b/blockdev.c > index a474bb5..824e718 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -1940,6 +1940,14 @@ void qmp_drive_backup(const char *device, const char *target, > } > } > > +BlockGraphNode * qmp_query_drive_graph(const char *device, Error **errp) Style: no space after * > +{ > + /* the implementation of this function would recurse through the > + * BlockDriverState graph to build it's result > + */ > + return NULL; Shouldn't you set errp when returning failure? > +++ b/qapi-schema.json > @@ -2008,6 +2008,38 @@ > { 'command': 'drive-backup', 'data': 'DriveBackup' } > > ## > +# @BlockGraphNode > +# > +# Information about a node of the block driver state graph > +# > +# @node-name: the name of the node in the graph > +# > +# @drv: the name of the block format used by this node as described in > +# @BlockDeviceInfo. It would be nice if BlockDeviceInfo and BlockGraphNode used an enum rather than an open-coded string for this field. > +# > +# @children: a list of @BlockGraphNode being the children of this node s/being/that are/ > +## > +# @query-drive-graph > +# > +# Get the block driver states graph for a given drive > +# > +# @device: the name of the device to get the graph from > +# > +# Returns: the root @BlockGraphNode > +# > +# Since 1.8 > +## > +{ 'command': 'query-drive-graph', > + 'data': { 'device': 'str' }, > + 'returns': 'BlockGraphNode' } Am I correct that it will be possible to have named nodes that are not currently associated with any device? If so, how do we learn about those nodes? Would it be better to have a command that returns an array of structs for all known node roots, with an optional member describing which device owns that node root? Something like: # Represent a root of a block graph # @root: a named node forming a root of a node graph # @device: #optional device name that owns this root { 'type': 'BlockGraphRoot', 'data': { 'root': 'BlockGraphNode', '*device': 'str' } } # @query_drive-graphs # Returns an array of all node graph roots { 'command': 'query-drive-graphs', 'returns': [ 'BlockGraphRoot' ] } possibly with 'data':{'*device':'str'} to allow filtering to just a 1-element array based on the device name (although I'm not sure if providing the complexity of filtering is worth it).
Le Wednesday 04 Dec 2013 à 16:46:34 (-0700), Eric Blake a écrit : > On 12/03/2013 06:26 AM, Benoît Canet wrote: > > In addition to Fam's review, > > s/skeletton/skeleton/ in subject > > > --- > > blockdev.c | 8 ++++++++ > > qapi-schema.json | 32 ++++++++++++++++++++++++++++++++ > > 2 files changed, 40 insertions(+) > > > > diff --git a/blockdev.c b/blockdev.c > > index a474bb5..824e718 100644 > > --- a/blockdev.c > > +++ b/blockdev.c > > @@ -1940,6 +1940,14 @@ void qmp_drive_backup(const char *device, const char *target, > > } > > } > > > > +BlockGraphNode * qmp_query_drive_graph(const char *device, Error **errp) > > Style: no space after * > > > +{ > > + /* the implementation of this function would recurse through the > > + * BlockDriverState graph to build it's result > > + */ > > + return NULL; > > Shouldn't you set errp when returning failure? > > > +++ b/qapi-schema.json > > @@ -2008,6 +2008,38 @@ > > { 'command': 'drive-backup', 'data': 'DriveBackup' } > > > > ## > > +# @BlockGraphNode > > +# > > +# Information about a node of the block driver state graph > > +# > > +# @node-name: the name of the node in the graph > > +# > > +# @drv: the name of the block format used by this node as described in > > +# @BlockDeviceInfo. > > It would be nice if BlockDeviceInfo and BlockGraphNode used an enum > rather than an open-coded string for this field. > > > +# > > +# @children: a list of @BlockGraphNode being the children of this node > > s/being/that are/ > > > +## > > +# @query-drive-graph > > +# > > +# Get the block driver states graph for a given drive > > +# > > +# @device: the name of the device to get the graph from > > +# > > +# Returns: the root @BlockGraphNode > > +# > > +# Since 1.8 > > +## > > +{ 'command': 'query-drive-graph', > > + 'data': { 'device': 'str' }, > > + 'returns': 'BlockGraphNode' } > > Am I correct that it will be possible to have named nodes that are not > currently associated with any device? If so, how do we learn about > those nodes? Would it be better to have a command that returns an array > of structs for all known node roots, with an optional member describing > which device owns that node root? Something like: The code have a list of all named nodes but not a list of named nodes roots. Also it's difficult to get the device name for a named node because the bses don't have any backward pointers to their parents. It could be done by recursing into all the blockbackend bs but it's twisted. In fact I am wondering if we really need something to spit out the named nodes topology in QMP for the simple reason that the names of the nodes are given by the management so the management should already know the topology. Best regards Benoît > > # Represent a root of a block graph > # @root: a named node forming a root of a node graph > # @device: #optional device name that owns this root > { 'type': 'BlockGraphRoot', > 'data': { 'root': 'BlockGraphNode', > '*device': 'str' } } > > # @query_drive-graphs > # Returns an array of all node graph roots > { 'command': 'query-drive-graphs', > 'returns': [ 'BlockGraphRoot' ] } > > possibly with 'data':{'*device':'str'} to allow filtering to just a > 1-element array based on the device name (although I'm not sure if > providing the complexity of filtering is worth it). > > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org >
On 12/05/2013 07:24 AM, Benoît Canet wrote: >> >> Am I correct that it will be possible to have named nodes that are not >> currently associated with any device? If so, how do we learn about >> those nodes? Would it be better to have a command that returns an array >> of structs for all known node roots, with an optional member describing >> which device owns that node root? Something like: > > The code have a list of all named nodes but not a list of named nodes roots. > Also it's difficult to get the device name for a named node because the bses don't > have any backward pointers to their parents. > It could be done by recursing into all the blockbackend bs but it's twisted. Still worth thinking about how to structure things so we could add it in the future if it turns out to be useful to management, but I can understand why you aren't providing it right away. > > In fact I am wondering if we really need something to spit out the named nodes > topology in QMP for the simple reason that the names of the nodes are given by the > management so the management should already know the topology. There's one case where management might not know - if libvirtd gets restarted while in the middle of an operation that was attempting to create a named node, then on restart and reconnection to the monitor, libvirt would want to query to see if the node actually got created or if the command needs to be attempted again. I'm not a fan of write-only interfaces - and making management responsible to track all named nodes with no way to query if qemu actually agrees with the topology that management thinks it has commanded feels like a write-only interface.
Le Thursday 05 Dec 2013 à 07:38:37 (-0700), Eric Blake a écrit : > On 12/05/2013 07:24 AM, Benoît Canet wrote: > > >> > >> Am I correct that it will be possible to have named nodes that are not > >> currently associated with any device? If so, how do we learn about > >> those nodes? Would it be better to have a command that returns an array > >> of structs for all known node roots, with an optional member describing > >> which device owns that node root? Something like: > > > > The code have a list of all named nodes but not a list of named nodes roots. > > Also it's difficult to get the device name for a named node because the bses don't > > have any backward pointers to their parents. > > It could be done by recursing into all the blockbackend bs but it's twisted. > > Still worth thinking about how to structure things so we could add it in > the future if it turns out to be useful to management, but I can > understand why you aren't providing it right away. > > > > > In fact I am wondering if we really need something to spit out the named nodes > > topology in QMP for the simple reason that the names of the nodes are given by the > > management so the management should already know the topology. > > There's one case where management might not know - if libvirtd gets > restarted while in the middle of an operation that was attempting to > create a named node, then on restart and reconnection to the monitor, > libvirt would want to query to see if the node actually got created or > if the command needs to be attempted again. I'm not a fan of write-only > interfaces - and making management responsible to track all named nodes > with no way to query if qemu actually agrees with the topology that > management thinks it has commanded feels like a write-only interface. Would a command returning info about a specific named node be sufficient for libvirt checks ? It's far less complex to implement than exposing the whole graph. We could also provide a simple command to list the names of the named nodes. Best regards Benoît > > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org >
On 12/05/2013 07:43 AM, Benoît Canet wrote: >> There's one case where management might not know - if libvirtd gets >> restarted while in the middle of an operation that was attempting to >> create a named node, then on restart and reconnection to the monitor, >> libvirt would want to query to see if the node actually got created or >> if the command needs to be attempted again. I'm not a fan of write-only >> interfaces - and making management responsible to track all named nodes >> with no way to query if qemu actually agrees with the topology that >> management thinks it has commanded feels like a write-only interface. > > Would a command returning info about a specific named node be sufficient for > libvirt checks ? > It's far less complex to implement than exposing the whole graph. > We could also provide a simple command to list the names of the named nodes. Yes, both of those ideas are useful; it still means management must track the topology between the nodes, but it at least gives management enough control to know which set of nodes exist to confirm which operations have occurred.
Le Thursday 05 Dec 2013 à 07:59:22 (-0700), Eric Blake a écrit : > On 12/05/2013 07:43 AM, Benoît Canet wrote: > > >> There's one case where management might not know - if libvirtd gets > >> restarted while in the middle of an operation that was attempting to > >> create a named node, then on restart and reconnection to the monitor, > >> libvirt would want to query to see if the node actually got created or > >> if the command needs to be attempted again. I'm not a fan of write-only > >> interfaces - and making management responsible to track all named nodes > >> with no way to query if qemu actually agrees with the topology that > >> management thinks it has commanded feels like a write-only interface. > > > > Would a command returning info about a specific named node be sufficient for > > libvirt checks ? > > It's far less complex to implement than exposing the whole graph. > > We could also provide a simple command to list the names of the named nodes. > > Yes, both of those ideas are useful; it still means management must > track the topology between the nodes, but it at least gives management > enough control to know which set of nodes exist to confirm which > operations have occurred. I will start by implementing the named nodes name list first. Don't worry for the other solution: my customer want crypto filters and multi BlockDriver state throttling as a filter so I will continue to enable filter in a progressive way. Best regards Benoît > > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org >
diff --git a/blockdev.c b/blockdev.c index a474bb5..824e718 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1940,6 +1940,14 @@ void qmp_drive_backup(const char *device, const char *target, } } +BlockGraphNode * qmp_query_drive_graph(const char *device, Error **errp) +{ + /* the implementation of this function would recurse through the + * BlockDriverState graph to build it's result + */ + return NULL; +} + #define DEFAULT_MIRROR_BUF_SIZE (10 << 20) void qmp_drive_mirror(const char *device, const char *target, diff --git a/qapi-schema.json b/qapi-schema.json index 8630eb5..938f8b9 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -2008,6 +2008,38 @@ { 'command': 'drive-backup', 'data': 'DriveBackup' } ## +# @BlockGraphNode +# +# Information about a node of the block driver state graph +# +# @node-name: the name of the node in the graph +# +# @drv: the name of the block format used by this node as described in +# @BlockDeviceInfo. +# +# @children: a list of @BlockGraphNode being the children of this node +# +# Since 1.8 +## +{ 'type': 'BlockGraphNode', + 'data': { 'node-name': 'str', 'drv': 'str', 'children': ['BlockGraphNode'] } } + +## +# @query-drive-graph +# +# Get the block driver states graph for a given drive +# +# @device: the name of the device to get the graph from +# +# Returns: the root @BlockGraphNode +# +# Since 1.8 +## +{ 'command': 'query-drive-graph', + 'data': { 'device': 'str' }, + 'returns': 'BlockGraphNode' } + +## # @drive-mirror # # Start mirroring a block device's writes to a new destination.